Windows runner: fix LSP editor tests #1312
Conversation
WalkthroughReplaces a reconfigure-based synchronous test setup with explicit project setup, a UI-triggered build, a build-completion wait, and an asynchronous post-build wait; cleans up imports/assertions and formatting in a Clang test; increases tab wait time and adds descriptive assertion messages in the SDKconfig test; changes CTab wait logic to treat existence (not activation) as success and handle missing tabs without throwing. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant T as Test.setupEnv
participant P as ProjectOps
participant U as UI (Context Menu)
participant W as WaitUtility
rect rgba(220,240,220,0.30)
T->>P: setupProject(...)
T->>U: buildProjectUsingContextMenu(...)
U-->>P: trigger build
T->>P: waitForProjectBuild(...)
T->>W: waitForOperationsInProgressToFinishAsync(...)
end
Note over T,P: replaces previous reconfigure + sync wait
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ 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). (3)
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: 1
🧹 Nitpick comments (3)
tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/executable/cases/project/IDFProjectLspCdtFunctionalityTest.java (3)
51-54: Ensure reconfigure is deterministically disabled in setupProject().setupProject() currently toggles the checkbox with click(); if the default changes, it may re‑enable idf.py reconfigure and reintroduce the lingering process. Prefer explicit deselect().
In ProjectTestOperations.setupProject(...):
// replace the .click() with an explicit deselect SWTBotCheckBox reconfig = bot.checkBox("Run idf.py reconfigure after project creation to initialize the CMake build configuration"); if (reconfig.isChecked()) { reconfig.deselect(); // ensure idf.py reconfigure will NOT run }Optional: assert the state after deselect() and log it for CI diagnostics.
54-54: Async wait may return before indexer/LSP jobs complete.If flakiness persists, consider switching to the sync variant here or add a targeted wait for CDT indexer/LSP idle before opening files.
76-76: Replace fixed sleeps with condition-based waits.Use TestWidgetWaitUtility to wait for:
- proposals list to become non-empty,
- Problems view rowCount change,
- styled ranges presence for highlighting.
Also applies to: 112-112, 124-124, 138-138
📜 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/IDFProjectLspCdtFunctionalityTest.java(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/executable/cases/project/IDFProjectLspCdtFunctionalityTest.java (1)
tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/operations/ProjectTestOperations.java (1)
ProjectTestOperations(51-785)
⏰ 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
| ProjectTestOperations.setupProjectWithReconfigureCommand(PROJECT_NAME, "EspressIf", "Espressif IDF Project", //$NON-NLS-1$ //$NON-NLS-2$ | ||
| bot); | ||
| TestWidgetWaitUtility.waitForOperationsInProgressToFinishSync(bot); | ||
| ProjectTestOperations.setupProject(PROJECT_NAME, "EspressIf", "Espressif IDF Project", bot); |
There was a problem hiding this comment.
Fix category label typo ("EspressIf" → "Espressif").
This will fail to locate the wizard node and break project setup.
- ProjectTestOperations.setupProject(PROJECT_NAME, "EspressIf", "Espressif IDF Project", bot);
+ ProjectTestOperations.setupProject(PROJECT_NAME, "Espressif", "Espressif IDF Project", bot);📝 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.
| ProjectTestOperations.setupProject(PROJECT_NAME, "EspressIf", "Espressif IDF Project", bot); | |
| ProjectTestOperations.setupProject(PROJECT_NAME, "Espressif", "Espressif IDF Project", bot); |
🤖 Prompt for AI Agents
In
tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/executable/cases/project/IDFProjectLspCdtFunctionalityTest.java
around line 51, the wizard category label is misspelled as "EspressIf" causing
the wizard node lookup to fail; update the string literal to "Espressif" in the
ProjectTestOperations.setupProject(...) call so the correct category name is
used.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (8)
tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/executable/cases/project/NewEspressifIDFProjectClangFilesTest.java (8)
7-7: Good switch to assertEquals import. Also unify equals checks.Since assertEquals is imported, prefer it over assertTrue(expected.equals(actual)) below for clearer diffs.
Apply this diff at Lines 421-426:
- assertTrue(expectedClangdPath.equals(actualClangdPath)); + assertEquals(expectedClangdPath, actualClangdPath);
188-199: Key change: setupProject + explicit waits is the right fix for Windows hogging.Nice replacement of idf.py reconfigure with setupProject and a sync wait. Consider removing the fixed sleep to reduce flakiness.
Apply this diff:
- bot.sleep(1000); + // Rely on explicit waits above; avoid fixed sleeps
257-267: Use of IDFUtil.getBuildDir is correct; improve naming and null safety.
- Method name should be getExpectedBuildFolderPath (camelCase), not PATH.
- If IDFUtil.getBuildDir(project) returns null, Paths.get(null) will throw NPE. Add a guard and a clearer error.
Example fix:
- private static String getExpectedBuildFolderPATH(String projectName) throws IOException + private static String getExpectedBuildFolderPath(String projectName) throws IOException { try { IProject project = ResourcesPlugin.getWorkspace().getRoot().getProject(projectName); - String buildFolder = IDFUtil.getBuildDir(project); + String buildFolder = IDFUtil.getBuildDir(project); + if (buildFolder == null || buildFolder.isBlank()) { + throw new IOException("Build directory is not configured for project: " + projectName); + } Path buildFolderPath = Paths.get(buildFolder); return buildFolderPath.toAbsolutePath().toString(); }Also update call sites accordingly (e.g., Line 279).
397-410: Avoid duplication with build-folder helpers.This method duplicates logic from getExpectedBuildFolderPATH. Refactor to reuse it and only prepend the flag.
Apply this diff:
- private static String getExpectedBuildFolderPATHforClangdAdditionalArgument(String projectName) - throws IOException - { - try - { - IProject project = ResourcesPlugin.getWorkspace().getRoot().getProject(projectName); - String buildFolder = IDFUtil.getBuildDir(project); - Path buildFolderPath = Paths.get(buildFolder); - return "--compile-commands-dir=" + buildFolderPath.toAbsolutePath().toString(); - } - catch (CoreException e) - { - throw new IOException("Failed to get build directory for project: " + projectName, e); - } - } + private static String getExpectedBuildFolderPathForClangdAdditionalArgument(String projectName) + throws IOException + { + try { + String buildPath = getExpectedBuildFolderPath(projectName); + return "--compile-commands-dir=" + buildPath; + } catch (IOException e) { + throw e; + } + }And update the call at Line 424 accordingly.
413-418: Handle missing toolchain path to avoid NPE.IDFUtil.getToolchainExePathForActiveTarget() can return null; Paths.get(null) will NPE. Fail fast with a clear message.
Apply this diff:
- String toolchain = IDFUtil.getToolchainExePathForActiveTarget(); + String toolchain = IDFUtil.getToolchainExePathForActiveTarget(); + if (toolchain == null || toolchain.isBlank()) { + throw new IOException("Active toolchain not found. Ensure a launch target is selected and toolchain is installed."); + }
421-426: Prefer assertEquals for clarity and consistent failure output.Use assertEquals as elsewhere in this class.
- assertTrue(expectedClangdPath.equals(actualClangdPath)); + assertEquals(expectedClangdPath, actualClangdPath);
454-459: Close Preferences robustly across platform variations.Button labels vary ("Apply and Close", "Close", "Cancel"). Consider a small helper that clicks the first available among these, then waits.
Example:
- preferencesShell.bot().button("Cancel").click(); // Or "Apply and Close" + if (preferencesShell.bot().button("Apply and Close").isEnabled()) { + preferencesShell.bot().button("Apply and Close").click(); + } else if (preferencesShell.bot().button("Close").isEnabled()) { + preferencesShell.bot().button("Close").click(); + } else { + preferencesShell.bot().button("Cancel").click(); + }
461-467: Wait for background operations after target change.After clicking “Yes”, jobs may start (index rebuild, toolchain detection). Add an explicit async wait to stabilize downstream assertions.
- preferencesShell.bot().button("Yes").click(); // Or "Apply and Close" - TestWidgetWaitUtility.waitWhileDialogIsVisible(bot, "IDF Launch Target Changed", 10000); + preferencesShell.bot().button("Yes").click(); + TestWidgetWaitUtility.waitWhileDialogIsVisible(bot, "IDF Launch Target Changed", 10000); + TestWidgetWaitUtility.waitForOperationsInProgressToFinishAsync(bot);
📜 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(8 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)
bundles/com.espressif.idf.core/src/com/espressif/idf/core/util/IDFUtil.java (1)
IDFUtil(59-996)
⏰ 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_windows
- GitHub Check: build
🔇 Additional comments (5)
tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/executable/cases/project/NewEspressifIDFProjectClangFilesTest.java (5)
116-117: Test reads well; scenario name is clear.No issues spotted here.
128-129: Coverage for target/driver updates looks good.Name reflects behavior; fine as is.
295-303: Text block for expected .clang-format content looks fine.No functional concerns.
310-320: Confirm clang-format accepts zero widths in your CI toolchain.IndentWidth: 0 and TabWidth: 0 may be version-dependent. Please confirm the clang-format version used by the Windows runner supports these values to avoid flaky formatting results.
Would you like me to add a small pre-check in the test harness to log clang-format --version before running these assertions?
429-434: Nice improvement: assertEquals on Drivers field.Clearer diffs on failure.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/executable/cases/project/NewEspressifIDFProjectSDKconfigTest.java (1)
186-195: Deduplicate magic timeout: extract a single constant.Avoid hardcoding 15000 in multiple places; define a constant and use it in both calls.
Add near the top of Fixture:
private static final int CTAB_APPEAR_TIMEOUT_MS = 15000;Apply this diff to both call sites:
- TestWidgetWaitUtility.waitForCTabToAppear(bot, "SDK Configuration (sdkconfig)", 15000); + TestWidgetWaitUtility.waitForCTabToAppear(bot, "SDK Configuration (sdkconfig)", CTAB_APPEAR_TIMEOUT_MS);
📜 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/NewEspressifIDFProjectSDKconfigTest.java(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/executable/cases/project/NewEspressifIDFProjectSDKconfigTest.java (1)
tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/operations/ProjectTestOperations.java (1)
ProjectTestOperations(51-785)
⏰ 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_windows
🔇 Additional comments (2)
tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/executable/cases/project/NewEspressifIDFProjectSDKconfigTest.java (2)
183-188: Bump to 15s reduces Windows flakiness — LGTM.Good call extending the CTab wait after double‑click open; this should mitigate slow UI on Windows runners.
Please confirm this eliminated the sporadic timeout on your Windows CI run for this test.
190-195: Same here — LGTM for context‑menu path.Matching the 15s timeout for the context‑menu open keeps both paths consistent.
If any residual flakes remain, consider capturing timing metrics to tune further.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 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/NewEspressifIDFProjectSDKconfigTest.java(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/executable/cases/project/NewEspressifIDFProjectSDKconfigTest.java (1)
tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/operations/ProjectTestOperations.java (1)
ProjectTestOperations(51-785)
⏰ 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_windows
| bot.sleep(5000); | ||
| bot.cTabItem("SDK Configuration (sdkconfig)").activate(); | ||
| } |
There was a problem hiding this comment.
Replace fixed sleep with a deterministic wait for the editor tab
A hard-coded 5s sleep is flaky across runners. Wait explicitly for the tab to exist, then activate it.
Apply this diff:
- bot.sleep(5000);
- bot.cTabItem("SDK Configuration (sdkconfig)").activate();
+ bot.waitUntil(new org.eclipse.swtbot.swt.finder.waits.DefaultCondition() {
+ @Override public boolean test() {
+ try { bot.cTabItem("SDK Configuration (sdkconfig)"); return true; }
+ catch (org.eclipse.swtbot.swt.finder.exceptions.WidgetNotFoundException e) { return false; }
+ }
+ @Override public String getFailureMessage() { return "SDK Configuration tab did not appear."; }
+ }, 120000);
+ bot.cTabItem("SDK Configuration (sdkconfig)").activate();Optionally add imports instead of FQCNs:
import org.eclipse.swtbot.swt.finder.waits.DefaultCondition;
import org.eclipse.swtbot.swt.finder.exceptions.WidgetNotFoundException;We can also centralize this into TestWidgetWaitUtility.waitForAndActivateCTab(label, timeout) and reuse it here and below. Happy to draft it.
🤖 Prompt for AI Agents
In
tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/executable/cases/project/NewEspressifIDFProjectSDKconfigTest.java
around lines 187-189, replace the hard-coded bot.sleep(5000) with a
deterministic wait that waits for the CTabItem titled "SDK Configuration
(sdkconfig)" to appear and then activates it; implement this by using SWTBot's
wait mechanism (create a DefaultCondition that returns true when
bot.cTabItem("SDK Configuration (sdkconfig)") exists/does not throw
WidgetNotFoundException) with a sensible timeout (e.g., 10_000 ms) and then call
activate(), add the necessary imports
(org.eclipse.swtbot.swt.finder.waits.DefaultCondition and
org.eclipse.swtbot.swt.finder.exceptions.WidgetNotFoundException) or keep the
FQCNs, and consider refactoring into a reusable
TestWidgetWaitUtility.waitForAndActivateCTab(label, timeout) for reuse in this
and later occurrences.
| bot.sleep(5000); | ||
| bot.cTabItem("SDK Configuration (sdkconfig)").activate(); | ||
| } |
There was a problem hiding this comment.
Same here: avoid fixed 5s sleep; wait until the tab appears
Mirror the deterministic wait for the context-menu path to remove flakiness.
Apply this diff:
- bot.sleep(5000);
- bot.cTabItem("SDK Configuration (sdkconfig)").activate();
+ bot.waitUntil(new org.eclipse.swtbot.swt.finder.waits.DefaultCondition() {
+ @Override public boolean test() {
+ try { bot.cTabItem("SDK Configuration (sdkconfig)"); return true; }
+ catch (org.eclipse.swtbot.swt.finder.exceptions.WidgetNotFoundException e) { return false; }
+ }
+ @Override public String getFailureMessage() { return "SDK Configuration tab did not appear."; }
+ }, 120000);
+ bot.cTabItem("SDK Configuration (sdkconfig)").activate();📝 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.
| bot.sleep(5000); | |
| bot.cTabItem("SDK Configuration (sdkconfig)").activate(); | |
| } | |
| bot.waitUntil(new org.eclipse.swtbot.swt.finder.waits.DefaultCondition() { | |
| @Override public boolean test() { | |
| try { bot.cTabItem("SDK Configuration (sdkconfig)"); return true; } | |
| catch (org.eclipse.swtbot.swt.finder.exceptions.WidgetNotFoundException e) { return false; } | |
| } | |
| @Override public String getFailureMessage() { return "SDK Configuration tab did not appear."; } | |
| }, 120000); | |
| bot.cTabItem("SDK Configuration (sdkconfig)").activate(); | |
| } |
🤖 Prompt for AI Agents
In
tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/executable/cases/project/NewEspressifIDFProjectSDKconfigTest.java
around lines 195-197, replace the hard-coded bot.sleep(5000) with a
deterministic wait for the "SDK Configuration (sdkconfig)" CTabItem to appear
and be enabled; remove the sleep call and use SWTBot's waitUntil with a
condition such as Conditions.widgetIsEnabled(new DefaultCTabItem("SDK
Configuration (sdkconfig)")) (optionally supplying a timeout), then call
bot.cTabItem("SDK Configuration (sdkconfig)").activate() once the wait succeeds.
Description
LSP editor tests were hogging up the windows runner.
Used setupProject method to avoid using 'idf.py reconfigure', because process was still up after execution completed.
Type of change
Please delete options that are not relevant.
How has this been tested?
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration
Test Configuration:
Checklist
Summary by CodeRabbit