fix: updated Clangd test case based on latest changes #1338
Conversation
WalkthroughUpdated a UI test: renamed the main test and helper methods, removed LaunchBar target-selector and target-change/build flows, and replaced per-target driver assertions with direct clangd path and query-driver assertions. Changes
Sequence Diagram(s)(omitted — changes are test refactor only, no runtime control-flow changes warranting a diagram) Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧬 Code graph analysis (1)tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/executable/cases/project/NewEspressifIDFProjectClangFilesTest.java (2)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
🔇 Additional comments (3)
Tip 📝 Customizable high-level summaries are now available in beta!You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.
Example instruction:
Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/executable/cases/project/NewEspressifIDFProjectClangFilesTest.java (1)
126-131: Consider renaming the test to better reflect its behavior.The test method name
givenNewProjectIsCreatedThenClangdAndQueryDriversPathsAreUpdatedimplies verification that an update occurred, but the test only checks the current state of the paths without verifying that any update action triggered them. Consider renaming togivenNewProjectIsCreatedThenClangdAndQueryDriversPathsAreSetor similar to more accurately reflect the test's behavior.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/executable/cases/project/NewEspressifIDFProjectClangFilesTest.java(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/executable/cases/project/NewEspressifIDFProjectClangFilesTest.java (1)
tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/executable/cases/project/NewEspressifIDFProjectTest.java (1)
Fixture(193-444)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build
- GitHub Check: build_macos
🔇 Additional comments (3)
tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/executable/cases/project/NewEspressifIDFProjectClangFilesTest.java (3)
126-131: Verify whether clangd behavior on target changes needs test coverage.The test simplification is confirmed: it no longer builds the project or performs explicit target selection using
LaunchBarTargetSelector. The method namesthenClangdPathUpdateOnSelectedTarget()andthenQueryDriversUpdateOnSelectedTarget()are misleading—they merely verify that preferences contain expected values after project creation, not that paths update when targets change.If clangd path updates on target selection is a supported feature, this test coverage gap should be addressed. No evidence in the codebase suggests target-change behavior is tested for clangd elsewhere.
434-439: Use "OK" or "Apply and Close" instead of "Cancel" for closing a read-only preferences dialog.The button was changed from "Apply and Close" to "Cancel". While this works functionally since no preferences are modified at that point (the method only reads values after
thenCompare*operations), semantically "Cancel" implies discarding changes. The standard pattern in this codebase is "Apply and Close" (as seen insetupAutoSave()). Even for read-only closes, use "OK" or "Apply and Close" to avoid confusion for future maintainers.
126-131: Verify that clangd and query driver paths are initialized without a build or explicit target selection.The test creates a project and checks IDE preferences for paths, but doesn't explicitly build the project or select a target (despite method names suggesting "OnSelectedTarget"). According to Espressif documentation, these paths are "configured based on the selected target and the project you're building." Verify that:
- Paths are populated with expected values immediately after project creation/selection alone
- No build or target selection is required for this verification to pass
- The method names accurately reflect what the test does
If paths require a build or explicit target selection, add these operations to the test.
| private static void thenClangdPathUpdateOnSelectedTarget() throws Exception | ||
| { | ||
| whenOpenClangdPreferences(); | ||
| thenCompareActualClangdDriversWithExpected("NewProjectClangFilesTest6"); | ||
| closePreferencesDialog(); | ||
| thenCompareActualClangdPathWithExpected("NewProjectClangFilesTest6"); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Use the projectName field instead of hardcoding the project name.
Line 188 hardcodes "NewProjectClangFilesTest6" instead of using the projectName field available in the Fixture class. This makes the code brittle and violates the DRY principle.
Apply this diff to use the field:
- thenCompareActualClangdPathWithExpected("NewProjectClangFilesTest6");
+ thenCompareActualClangdPathWithExpected(projectName);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| private static void thenClangdPathUpdateOnSelectedTarget() throws Exception | |
| { | |
| whenOpenClangdPreferences(); | |
| thenCompareActualClangdDriversWithExpected("NewProjectClangFilesTest6"); | |
| closePreferencesDialog(); | |
| thenCompareActualClangdPathWithExpected("NewProjectClangFilesTest6"); | |
| } | |
| private static void thenClangdPathUpdateOnSelectedTarget() throws Exception | |
| { | |
| whenOpenClangdPreferences(); | |
| thenCompareActualClangdPathWithExpected(projectName); | |
| } |
🤖 Prompt for AI Agents
In
tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/executable/cases/project/NewEspressifIDFProjectClangFilesTest.java
around lines 185–189, the call
thenCompareActualClangdPathWithExpected("NewProjectClangFilesTest6") uses a
hardcoded project name; replace the string literal with the projectName field
from the Fixture class (e.g.
thenCompareActualClangdPathWithExpected(Fixture.projectName)) so the test uses
the shared fixture value instead of a brittle hardcoded name.
| private static void thenQueryDriversUpdateOnSelectedTarget() throws Exception | ||
| { | ||
| Fixture.thenLaunchTargetIsSelectedFromLaunchTargets("esp32s2"); | ||
| Fixture.closeTargetDialog(); | ||
| thenCompareActualQueryDriversWithExpected("NewProjectClangFilesTest6"); | ||
| closePreferencesDialog(); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Use the projectName field instead of hardcoding the project name.
Line 193 hardcodes "NewProjectClangFilesTest6" instead of using the projectName field. This creates unnecessary coupling and makes the method less reusable.
Apply this diff to use the field:
- thenCompareActualQueryDriversWithExpected("NewProjectClangFilesTest6");
+ thenCompareActualQueryDriversWithExpected(projectName);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| private static void thenQueryDriversUpdateOnSelectedTarget() throws Exception | |
| { | |
| Fixture.thenLaunchTargetIsSelectedFromLaunchTargets("esp32s2"); | |
| Fixture.closeTargetDialog(); | |
| thenCompareActualQueryDriversWithExpected("NewProjectClangFilesTest6"); | |
| closePreferencesDialog(); | |
| } | |
| private static void thenQueryDriversUpdateOnSelectedTarget() throws Exception | |
| { | |
| thenCompareActualQueryDriversWithExpected(projectName); | |
| closePreferencesDialog(); | |
| } |
| private static void thenCompareActualQueryDriversWithExpected(String projectName) throws IOException | ||
| { | ||
| SWTBotShell prefrencesShell = bot.shell("Preferences"); | ||
| String actualQueryDriversPath = prefrencesShell.bot().textWithLabel("Drivers").getText(); | ||
| String expectedQueryDriversPath = "**"; | ||
| assertEquals(expectedQueryDriversPath, actualQueryDriversPath); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Remove unused parameter and clarify the expected value.
The method has two issues:
- The
projectNameparameter is declared but never used in the method body. - The expected value
"**"is a magic string without explanation. Add a comment or constant to clarify what this represents in the context of query drivers.
Apply this diff to remove the unused parameter:
- private static void thenCompareActualQueryDriversWithExpected(String projectName) throws IOException
+ private static void thenCompareActualQueryDriversWithExpected() throws IOException
{
SWTBotShell prefrencesShell = bot.shell("Preferences");
String actualQueryDriversPath = prefrencesShell.bot().textWithLabel("Drivers").getText();
- String expectedQueryDriversPath = "**";
+ // "**" matches all drivers in the query driver path
+ String expectedQueryDriversPath = "**";
assertEquals(expectedQueryDriversPath, actualQueryDriversPath);
}And update the call site at line 193:
- thenCompareActualQueryDriversWithExpected(projectName);
+ thenCompareActualQueryDriversWithExpected();Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In
tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/executable/cases/project/NewEspressifIDFProjectClangFilesTest.java
around lines 400-406, remove the unused projectName parameter from
thenCompareActualQueryDriversWithExpected, update its signature to take no
parameters, and update the caller at line 193 to call the method with no
argument; replace the magic string "**" with a named constant (e.g.,
EXPECTED_QUERY_DRIVERS) declared at class scope with a short comment explaining
what "**" signifies in the query drivers context, and use that constant in the
assertion.
| private static void thenCompareActualClangdPathWithExpected(String projectName) throws IOException | ||
| { | ||
| SWTBotShell prefrencesShell = bot.shell("Preferences"); | ||
| String actualDriversPath = prefrencesShell.bot().textWithLabel("Drivers").getText(); | ||
| String expectedDriversPath = "**"; | ||
| assertEquals(expectedDriversPath, actualDriversPath); | ||
| String actualClangdPath = prefrencesShell.bot().textWithLabel("Path").getText(); | ||
| String expectedClangdPath = "bin\\clangd"; | ||
| assertTrue(actualClangdPath.contains(expectedClangdPath)); | ||
| } |
There was a problem hiding this comment.
Fix platform-specific path separator and remove unused parameter.
The method has critical issues:
- Line 412 uses Windows-specific path separator
\\, which will fail on Linux and macOS. The PR indicates testing was done on all platforms, but this hardcoded separator breaks cross-platform compatibility. - The
projectNameparameter is unused.
Apply this diff to fix the path separator issue:
- private static void thenCompareActualClangdPathWithExpected(String projectName) throws IOException
+ private static void thenCompareActualClangdPathWithExpected() throws IOException
{
SWTBotShell prefrencesShell = bot.shell("Preferences");
String actualClangdPath = prefrencesShell.bot().textWithLabel("Path").getText();
- String expectedClangdPath = "bin\\clangd";
+ // Use platform-independent path matching
+ String expectedClangdPath = "clangd";
- assertTrue(actualClangdPath.contains(expectedClangdPath));
+ assertTrue("Clangd path should contain 'clangd': " + actualClangdPath,
+ actualClangdPath.contains(expectedClangdPath) && actualClangdPath.contains("bin"));
}And update the call site at line 188:
- thenCompareActualClangdPathWithExpected(projectName);
+ thenCompareActualClangdPathWithExpected();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| private static void thenCompareActualClangdPathWithExpected(String projectName) throws IOException | |
| { | |
| SWTBotShell prefrencesShell = bot.shell("Preferences"); | |
| String actualDriversPath = prefrencesShell.bot().textWithLabel("Drivers").getText(); | |
| String expectedDriversPath = "**"; | |
| assertEquals(expectedDriversPath, actualDriversPath); | |
| String actualClangdPath = prefrencesShell.bot().textWithLabel("Path").getText(); | |
| String expectedClangdPath = "bin\\clangd"; | |
| assertTrue(actualClangdPath.contains(expectedClangdPath)); | |
| } | |
| private static void thenCompareActualClangdPathWithExpected() throws IOException | |
| { | |
| SWTBotShell prefrencesShell = bot.shell("Preferences"); | |
| String actualClangdPath = prefrencesShell.bot().textWithLabel("Path").getText(); | |
| // Use platform-independent path matching | |
| String expectedClangdPath = "clangd"; | |
| assertTrue("Clangd path should contain 'clangd': " + actualClangdPath, | |
| actualClangdPath.contains(expectedClangdPath) && actualClangdPath.contains("bin")); | |
| } |
🤖 Prompt for AI Agents
In
tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/executable/cases/project/NewEspressifIDFProjectClangFilesTest.java
around lines 408-414, the helper thenCompareActualClangdPathWithExpected uses a
hardcoded Windows path separator ("bin\\clangd") and accepts an unused
projectName parameter; change the expected path to use File.separator (e.g.
"bin" + File.separator + "clangd") to be platform independent, remove the unused
projectName parameter from the method signature, and update its single call site
at line 188 to call the method without the removed argument.
AndriiFilippov
force-pushed
the
IEP-1640
branch
from
7092056 to
1b0a16a
Compare
AndriiFilippov
force-pushed
the
IEP-1640
branch
from
1b0a16a to
af90f61
Compare
Description
Added Clangd path and query driver paths verification in the Clangd preference page.
Fixes # (IEP-XXX)
Type of change
Please delete options that are not relevant.
Test Configuration:
Checklist
Summary by CodeRabbit
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.