SWTBot test case: Launch Target editor verification #1304
Conversation
WalkthroughAdds a new SWTBot UI test class for launch-target editor flows; tightens build-wait error handling in ProjectTestOperations; and enhances LaunchBarTargetSelector with presence-check, scrolling, and more robust selection logic. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Tester as SWTBot Test
participant Fixture
participant Env as EnvSetupOperations
participant ProjOps as ProjectTestOperations
participant LBT as LaunchBarTargetSelector
participant Dialog as "IDF Launch Target Changed"
Tester->>Fixture: beforeTestClass()
Fixture->>Env: load environment
Tester->>Fixture: run test
Fixture->>ProjOps: create project & build via context menu
alt build wait success
ProjOps-->>Fixture: build completed
else build wait fails
ProjOps-->>Fixture: throws AssertionError("Project Build failed")
end
Fixture->>LBT: selectNewLaunchTarget("esp32c2")
LBT-->>Fixture: open popup, locate/choose target
Fixture->>Dialog: wait for "IDF Launch Target Changed"
Dialog-->>Fixture: appears -> Fixture clicks "Yes"
Fixture->>ProjOps: refresh project & verify build folder cleaned
Tester->>Fixture: afterEachTest()
Fixture->>Env: cleanup
sequenceDiagram
autonumber
participant Caller as Test / User
participant LBT as LaunchBarTargetSelector
participant Popup as Target Popup (ScrolledComposite)
Caller->>LBT: isTargetPresent(text)
LBT->>Popup: open popup & inspect items
alt many items (long list)
LBT->>Popup: scrollToBottom + filter by Label text
Popup-->>LBT: filtered labels
LBT-->>Caller: return true if label matches
else few items (short list)
LBT->>Popup: query Label by exact text
Popup-->>LBT: widget (or not)
LBT-->>Caller: return match result
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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: 4
🧹 Nitpick comments (4)
tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/executable/cases/project/IDFProjectLaunchTargetEditorFunctionalityTest.java (4)
123-126: Add assert message for easier diagnosis on failureImproves debuggability in CI logs.
- assertTrue(ProjectTestOperations.findProjectFullCleanedFilesInBuildFolder(projectName, bot)); + assertTrue( + "Build folder not fully cleaned for project '" + projectName + + "' (expected no '.elf' and no 'bootloader').", + ProjectTestOperations.findProjectFullCleanedFilesInBuildFolder(projectName, bot));
77-83: Replace fixed sleep with deterministic waitAvoids timing-related flakes right after environment setup.
EnvSetupOperations.setupEspressifEnv(bot); - bot.sleep(1000); + TestWidgetWaitUtility.waitForOperationsInProgressToFinishSync(bot); ProjectTestOperations.deleteAllProjects(bot);
44-55: Improve cleanup error reportingPrint the stack trace to aid triage if teardown fails.
catch (Exception e) { - System.err.println("Error during cleanup: " + e.getMessage()); + System.err.println("Error during cleanup: " + e.getMessage()); + e.printStackTrace(); }
70-76: Optional: centralize hard-coded UI stringsConsider extracting literals (project name, target, dialog title, button labels) into constants to reduce drift.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/executable/cases/project/IDFProjectLaunchTargetEditorFunctionalityTest.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/IDFProjectLaunchTargetEditorFunctionalityTest.java (3)
tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/common/WorkBenchSWTBot.java (1)
WorkBenchSWTBot(14-28)tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/operations/EnvSetupOperations.java (1)
EnvSetupOperations(12-85)tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/operations/ProjectTestOperations.java (1)
ProjectTestOperations(51-776)
⏰ 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 (1)
tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/executable/cases/project/IDFProjectLaunchTargetEditorFunctionalityTest.java (1)
110-114: No changes required: UI strings are correct
The target id "esp32c2" and dialog title "IDF Launch Target Changed" exactly match the entries in esp-config.json and messages.properties.
...ssif/idf/ui/test/executable/cases/project/IDFProjectLaunchTargetEditorFunctionalityTest.java
Show resolved
Hide resolved
...ssif/idf/ui/test/executable/cases/project/IDFProjectLaunchTargetEditorFunctionalityTest.java
Outdated
Show resolved
Hide resolved
...ssif/idf/ui/test/executable/cases/project/IDFProjectLaunchTargetEditorFunctionalityTest.java
Show resolved
Hide resolved
...ssif/idf/ui/test/executable/cases/project/IDFProjectLaunchTargetEditorFunctionalityTest.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (4)
tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/executable/cases/project/IDFProjectLaunchTargetEditorFunctionalityTest.java (4)
62-62: Fix the typo in category name.Also applies to: 75-75
12-14: Add missing import for Conditions.
120-128: Improve dialog handling stability.
162-165: Add synchronization after project refresh.
🧹 Nitpick comments (5)
tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/executable/cases/project/IDFProjectLaunchTargetEditorFunctionalityTest.java (5)
9-9: Remove unused AWT import.The
java.awt.Buttonimport is not used anywhere in the code and should be removed.-import java.awt.Button;
58-69: Verify test method name accurately describes the behavior.The test method name
givenNewProjectCreatedBuiltWhenSelectNewTargetWhenPopUpAppearsThenBuildFolderDeletedSuccessfullysuggests the build folder is deleted, but based on the implementation logic, it appears to verify that the build folder is cleaned (not necessarily deleted). Consider renaming for clarity.-public void givenNewProjectCreatedBuiltWhenSelectNewTargetWhenPopUpAppearsThenBuildFolderDeletedSuccessfully() +public void givenNewProjectCreatedBuiltWhenSelectNewTargetWhenPopUpAppearsThenBuildFolderCleanedSuccessfully()
142-145: Fix inconsistent code formatting.The if statement has inconsistent formatting with unnecessary braces placement.
-if(bot.button("Next >").isEnabled()) - { - bot.button("Next >").click(); - } +if (bot.button("Next >").isEnabled()) { + bot.button("Next >").click(); +}
93-93: Remove arbitrary sleep call.The hardcoded
bot.sleep(1000)is a code smell in UI tests as it introduces unnecessary delays and doesn't guarantee the intended state is reached.EnvSetupOperations.setupEspressifEnv(bot); -bot.sleep(1000); ProjectTestOperations.deleteAllProjects(bot);
89-95: Add exception handling for environment setup.The
loadEnv()method can throw exceptions but doesn't handle them appropriately. If environment setup fails, subsequent tests will fail with unclear error messages.private static void loadEnv() throws Exception { - bot = WorkBenchSWTBot.getBot(); - EnvSetupOperations.setupEspressifEnv(bot); - bot.sleep(1000); - ProjectTestOperations.deleteAllProjects(bot); + try { + bot = WorkBenchSWTBot.getBot(); + EnvSetupOperations.setupEspressifEnv(bot); + ProjectTestOperations.deleteAllProjects(bot); + } catch (Exception e) { + System.err.println("Failed to setup test environment: " + e.getMessage()); + throw e; + } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/executable/cases/project/IDFProjectLaunchTargetEditorFunctionalityTest.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/IDFProjectLaunchTargetEditorFunctionalityTest.java (3)
tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/common/WorkBenchSWTBot.java (1)
WorkBenchSWTBot(14-28)tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/operations/EnvSetupOperations.java (1)
EnvSetupOperations(12-85)tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/operations/ProjectTestOperations.java (1)
ProjectTestOperations(51-776)
⏰ 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
...ssif/idf/ui/test/executable/cases/project/IDFProjectLaunchTargetEditorFunctionalityTest.java
Show resolved
Hide resolved
...ssif/idf/ui/test/executable/cases/project/IDFProjectLaunchTargetEditorFunctionalityTest.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/operations/ProjectTestOperations.java (1)
726-729: Wrong boolean operator causes false positives.This should fail when either dimension mismatches; use OR instead of AND.
- if (columns != 6 && rows != 3) + if (columns != 6 || rows != 3) { return false; }
🧹 Nitpick comments (4)
tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/operations/ProjectTestOperations.java (4)
91-96: Make failure more diagnostic; also catch AssertionError explicitly.Enrich the assertion message with timeout/console context and ensure AssertionError from the wait utility is wrapped consistently.
- try { - TestWidgetWaitUtility.waitUntilViewContains(bot, "Build complete", consoleView, - DefaultPropertyFetcher.getLongPropertyValue(DEFAULT_PROJECT_BUILD_WAIT_PROPERTY, 300000)); - } catch (Exception e) { - throw new AssertionError("Project Build failed", e); - } + try { + final long waitMs = DefaultPropertyFetcher + .getLongPropertyValue(DEFAULT_PROJECT_BUILD_WAIT_PROPERTY, 300000); + TestWidgetWaitUtility.waitUntilViewContains(bot, "Build complete", consoleView, waitMs); + } catch (AssertionError e) { + throw new AssertionError( + MessageFormat.format("Project build did not complete within {0} ms in console 'CDT Build Console' (expected text: 'Build complete').", + DefaultPropertyFetcher.getLongPropertyValue(DEFAULT_PROJECT_BUILD_WAIT_PROPERTY, 300000)), e); + } catch (Exception e) { + throw new AssertionError( + MessageFormat.format("Project build wait failed due to unexpected exception after {0} ms.", + DefaultPropertyFetcher.getLongPropertyValue(DEFAULT_PROJECT_BUILD_WAIT_PROPERTY, 300000)), e); + }
153-176: Off-by-one in retry loop.
<= maxAttemptsyields 3 attempts formaxAttempts = 2. If two attempts are intended, use<.- int maxAttempts = 2; - for (int attempt = 0; attempt <= maxAttempts; attempt++) + int maxAttempts = 2; + for (int attempt = 0; attempt < maxAttempts; attempt++)
189-212: Same off-by-one retry issue here.- int maxAttempts = 2; - for (int attempt = 0; attempt <= maxAttempts; attempt++) + int maxAttempts = 2; + for (int attempt = 0; attempt < maxAttempts; attempt++)
287-295: Typo in method name ("Existance").Consider renaming to
checkFolderExistenceAfterPythonCleanfor clarity (update call sites accordingly).- public static boolean checkFolderExistanceAfterPythonClean(File folderExists) + public static boolean checkFolderExistenceAfterPythonClean(File folderExists)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/executable/cases/project/IDFProjectLaunchTargetEditorFunctionalityTest.java(1 hunks)tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/operations/ProjectTestOperations.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/executable/cases/project/IDFProjectLaunchTargetEditorFunctionalityTest.java
🧰 Additional context used
🧬 Code graph analysis (1)
tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/operations/ProjectTestOperations.java (1)
tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/common/configs/DefaultPropertyFetcher.java (1)
DefaultPropertyFetcher(20-90)
🔇 Additional comments (1)
tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/operations/ProjectTestOperations.java (1)
86-97: Confirm console source for build logs.Build waits target "CDT Build Console". If some flows emit to "ESP-IDF Console", this could miss the signal.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (5)
tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/executable/cases/project/IDFProjectLaunchTargetEditorFunctionalityTest.java (5)
196-199: Consider waiting after Refresh to harden assertions.If
launchCommandUsingContextMenualready waits, ignore; otherwise add a sync wait.ProjectTestOperations.launchCommandUsingContextMenu(projectName, bot, "Refresh"); + TestWidgetWaitUtility.waitForOperationsInProgressToFinishSync(bot);Please confirm whether
launchCommandUsingContextMenualready joins background jobs in your version.
71-80: Add an assertion to validate build outcome.The test currently has no final check; it will pass even if the build didn’t produce expected artifacts.
Fixture.whenCreateNewLaunchTarget(); Fixture.whenProjectIsBuiltUsingContextMenu(); + // e.g., assert build completed by checking expected artifact/state + // Prefer a helper like ProjectTestOperations.assertProjectBuiltSuccessfully(projectName, bot) + // or verify known build outputs exist in the Project Explorer.
62-64: Fix category typo: wizard node won’t be found ("EspressIf" → "Espressif").This will cause the New Project wizard selection to fail.
- Fixture.givenNewEspressifIDFProjectIsSelected("EspressIf", "Espressif IDF Project"); + Fixture.givenNewEspressifIDFProjectIsSelected("Espressif", "Espressif IDF Project"); ... - Fixture.givenNewEspressifIDFProjectIsSelected("EspressIf", "Espressif IDF Project"); + Fixture.givenNewEspressifIDFProjectIsSelected("Espressif", "Espressif IDF Project"); ... - Fixture.givenNewEspressifIDFProjectIsSelected("EspressIf", "Espressif IDF Project"); + Fixture.givenNewEspressifIDFProjectIsSelected("Espressif", "Espressif IDF Project");Also applies to: 75-77, 86-88
132-140: Stabilize dialog handling after clicking “Yes”: wait for shell close and jobs to finish.Prevents flakiness due to racing the dialog closure and background updates.
SWTBotShell shell = bot.shell("IDF Launch Target Changed"); shell.setFocus(); bot.button("Yes").click(); + bot.waitUntil(Conditions.shellCloses(shell), 20000); + TestWidgetWaitUtility.waitForOperationsInProgressToFinishSync(bot);
12-21: Add missing import for Conditions used in waits.Required for the added shell-close waits.
import org.eclipse.swtbot.swt.finder.junit.SWTBotJunit4ClassRunner; import org.eclipse.swtbot.swt.finder.widgets.SWTBotShell; +import org.eclipse.swtbot.swt.finder.waits.Conditions; import org.junit.After;
🧹 Nitpick comments (5)
tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/executable/cases/project/IDFProjectLaunchTargetEditorFunctionalityTest.java (5)
144-144: Replace arbitrary sleeps with deterministic waits.Sleeps are brittle; wait for operations/jobs instead.
- bot.sleep(500); + TestWidgetWaitUtility.waitForOperationsInProgressToFinishSync(bot);- bot.sleep(500); + TestWidgetWaitUtility.waitForOperationsInProgressToFinishSync(bot);Also applies to: 158-158
125-131: Use sync wait after build to avoid post-build races.Prefer the sync variant to ensure all related jobs complete before proceeding.
ProjectTestOperations.buildProjectUsingContextMenu(projectName, bot); ProjectTestOperations.waitForProjectBuild(bot); - TestWidgetWaitUtility.waitForOperationsInProgressToFinishAsync(bot); + TestWidgetWaitUtility.waitForOperationsInProgressToFinishSync(bot);
101-107: Avoid sleep during env setup.Replace with an explicit wait to reduce intermittency.
EnvSetupOperations.setupEspressifEnv(bot); - bot.sleep(1000); + TestWidgetWaitUtility.waitForOperationsInProgressToFinishSync(bot); ProjectTestOperations.deleteAllProjects(bot);
201-204: Add assertion message for clarity.Improves failure diagnostics.
- assertTrue(ProjectTestOperations.findProjectFullCleanedFilesInBuildFolder(projectName, bot)); + assertTrue("Expected build folder cleanup after target change", + ProjectTestOperations.findProjectFullCleanedFilesInBuildFolder(projectName, bot));
45-56: Prefer logging the full exception during cleanup.Printing only the message hides the stack trace; use e.printStackTrace() or a logger.
- System.err.println("Error during cleanup: " + e.getMessage()); + e.printStackTrace();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/executable/cases/project/IDFProjectLaunchTargetEditorFunctionalityTest.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/IDFProjectLaunchTargetEditorFunctionalityTest.java (3)
tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/common/WorkBenchSWTBot.java (1)
WorkBenchSWTBot(14-28)tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/operations/EnvSetupOperations.java (1)
EnvSetupOperations(12-85)tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/operations/ProjectTestOperations.java (1)
ProjectTestOperations(51-780)
⏰ 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
| SWTBotShell shell = bot.shell("New ESP Target"); | ||
| shell.setFocus(); | ||
| bot.button("Delete").click(); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Also wait after deleting the launch target.
Ensures the edit dialog is closed and the workspace settled before verification.
SWTBotShell shell = bot.shell("New ESP Target");
shell.setFocus();
bot.button("Delete").click();
+ bot.waitUntil(Conditions.shellCloses(shell), 20000);
+ TestWidgetWaitUtility.waitForOperationsInProgressToFinishSync(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.
| SWTBotShell shell = bot.shell("New ESP Target"); | |
| shell.setFocus(); | |
| bot.button("Delete").click(); | |
| } | |
| SWTBotShell shell = bot.shell("New ESP Target"); | |
| shell.setFocus(); | |
| bot.button("Delete").click(); | |
| bot.waitUntil(Conditions.shellCloses(shell), 20000); | |
| TestWidgetWaitUtility.waitForOperationsInProgressToFinishSync(bot); | |
| } |
🤖 Prompt for AI Agents
In
tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/executable/cases/project/IDFProjectLaunchTargetEditorFunctionalityTest.java
around lines 151 to 154, after clicking the "Delete" button the test does not
wait for the "New ESP Target" dialog to close or for background workspace jobs
to settle; update the test to wait until the shell is closed (e.g., SWTBotShell
waitUntil/Condition for shell not active or bot.waitUntil with ShellCondition)
and then wait for workspace/job completion (e.g., waitForJobs or
Job.getJobManager().join/IDLE) before proceeding with verification so the dialog
is fully dismissed and the workspace is stable.
| private static void thenLaunchTargetDeletedSuccessfully() throws Exception | ||
| { | ||
| bot.sleep(500); | ||
| LaunchBarTargetSelector targetSelector = new LaunchBarTargetSelector(bot); | ||
| // ??? | ||
|
|
||
| } |
There was a problem hiding this comment.
Implement verification: ‘thenLaunchTargetDeletedSuccessfully’ is empty.
Without an assertion this test can pass even if deletion failed.
private static void thenLaunchTargetDeletedSuccessfully() throws Exception
{
- bot.sleep(500);
- LaunchBarTargetSelector targetSelector = new LaunchBarTargetSelector(bot);
- // ???
+ LaunchBarTargetSelector targetSelector = new LaunchBarTargetSelector(bot);
+ boolean deleted;
+ try {
+ // If selection succeeds, target still exists
+ targetSelector.selectTarget("target");
+ deleted = false;
+ } catch (Exception e) {
+ // Expected: cannot select deleted target
+ deleted = true;
+ }
+ assertTrue("Launch target 'target' should be deleted and not selectable", deleted);
}If LaunchBarTargetSelector can expose isTargetPresent(String), prefer asserting assertTrue(!isTargetPresent("target")). I can submit that helper if desired.
📝 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 thenLaunchTargetDeletedSuccessfully() throws Exception | |
| { | |
| bot.sleep(500); | |
| LaunchBarTargetSelector targetSelector = new LaunchBarTargetSelector(bot); | |
| // ??? | |
| } | |
| private static void thenLaunchTargetDeletedSuccessfully() throws Exception | |
| { | |
| LaunchBarTargetSelector targetSelector = new LaunchBarTargetSelector(bot); | |
| boolean deleted; | |
| try { | |
| // If selection succeeds, target still exists | |
| targetSelector.selectTarget("target"); | |
| deleted = false; | |
| } catch (Exception e) { | |
| // Expected: cannot select deleted target | |
| deleted = true; | |
| } | |
| assertTrue("Launch target 'target' should be deleted and not selectable", deleted); | |
| } |
🤖 Prompt for AI Agents
In
tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/executable/cases/project/IDFProjectLaunchTargetEditorFunctionalityTest.java
around lines 156-162, the helper thenLaunchTargetDeletedSuccessfully is empty so
the test never checks deletion; call LaunchBarTargetSelector (after the existing
sleep) to verify the target is gone and assert that deletion succeeded. If
LaunchBarTargetSelector provides isTargetPresent(String), use
Assert.assertFalse(targetSelector.isTargetPresent("target")); otherwise retrieve
the target list via the selector and assert it does not contain the target name
(or use Assert.assertTrue(!targetList.contains("target"))), so the test fails
when the launch target is still present.
AndriiFilippov
changed the title
|
@kolipakakondal @sigmaaa @alirana01 guys, please, review |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/executable/cases/project/IDFProjectLaunchTargetEditorFunctionalityTest.java(1 hunks)tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/operations/selectors/LaunchBarTargetSelector.java(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/executable/cases/project/IDFProjectLaunchTargetEditorFunctionalityTest.java
⏰ 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)
- GitHub Check: build_macos
- GitHub Check: build
- GitHub Check: build_windows
🔇 Additional comments (4)
tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/operations/selectors/LaunchBarTargetSelector.java (4)
11-12: LGTM on added import.Needed for the labels scan in the new method.
20-20: LGTM on import.No concerns.
22-22: LGTM on import.No concerns.
30-30: LGTM on import.No concerns.
|
|
||
| public boolean isTargetPresent(String text) | ||
| { | ||
| click(); | ||
|
|
||
| try | ||
| { | ||
| SWTBotShell swtBotShell = bot().shellWithId(LaunchBarWidgetIds.POPUP); | ||
| ScrolledComposite scrolledComposite = swtBotShell.bot().widget(widgetOfType(ScrolledComposite.class)); | ||
|
|
||
| int numberOfItemsInScrolledComp = syncExec(() -> | ||
| ((Composite) scrolledComposite.getChildren()[0]).getChildren().length | ||
| ); | ||
|
|
||
| if (numberOfItemsInScrolledComp > NUM_FOR_FILTER_POPUP) | ||
| { | ||
| swtBotShell.bot().text().setText(text); | ||
|
|
||
| List<? extends Widget> labels = swtBotShell.bot().widgets(widgetOfType(Label.class)); | ||
| for (Widget widget : labels) | ||
| { | ||
| String labelText = syncExec(() -> ((Label) widget).getText()); | ||
| if (labelText.equals(text)) | ||
| { | ||
| return true; | ||
| } | ||
| } | ||
| return false; | ||
| } | ||
| else | ||
| { | ||
| Widget itemToCheck = swtBotShell.bot().widget(withText(text)); | ||
| String labelText = syncExec(() -> ((Label) itemToCheck).getText()); | ||
| return labelText.equals(text); | ||
| } | ||
| } | ||
| catch (WidgetNotFoundException e) | ||
| { | ||
| return false; | ||
| } | ||
| catch (Exception e) | ||
| { | ||
| e.printStackTrace(); | ||
| return false; | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Close the popup and avoid side effects; tighten matching scope to reduce false positives.
- The popup remains open on all return paths, and the filter text may remain populated. This can make subsequent SWTBot steps flaky. Ensure the shell is closed (e.g., in finally).
- After filtering, scanning all labels in the shell risks matching unrelated labels. Limit search to the ScrolledComposite’s list content.
- In the short-list branch, you can rely on label(text) instead of casting from a generic Widget.
Apply this diff:
- public boolean isTargetPresent(String text)
- {
- click();
-
- try
- {
- SWTBotShell swtBotShell = bot().shellWithId(LaunchBarWidgetIds.POPUP);
- ScrolledComposite scrolledComposite = swtBotShell.bot().widget(widgetOfType(ScrolledComposite.class));
-
- int numberOfItemsInScrolledComp = syncExec(() ->
- ((Composite) scrolledComposite.getChildren()[0]).getChildren().length
- );
-
- if (numberOfItemsInScrolledComp > NUM_FOR_FILTER_POPUP)
- {
- swtBotShell.bot().text().setText(text);
-
- List<? extends Widget> labels = swtBotShell.bot().widgets(widgetOfType(Label.class));
- for (Widget widget : labels)
- {
- String labelText = syncExec(() -> ((Label) widget).getText());
- if (labelText.equals(text))
- {
- return true;
- }
- }
- return false;
- }
- else
- {
- Widget itemToCheck = swtBotShell.bot().widget(withText(text));
- String labelText = syncExec(() -> ((Label) itemToCheck).getText());
- return labelText.equals(text);
- }
- }
- catch (WidgetNotFoundException e)
- {
- return false;
- }
- catch (Exception e)
- {
- e.printStackTrace();
- return false;
- }
- }
+ public boolean isTargetPresent(String text)
+ {
+ click();
+ SWTBotShell swtBotShell = bot().shellWithId(LaunchBarWidgetIds.POPUP);
+ boolean found = false;
+ try
+ {
+ ScrolledComposite scrolledComposite = swtBotShell.bot().widget(widgetOfType(ScrolledComposite.class));
+ int numberOfItemsInScrolledComp = syncExec(() ->
+ ((Composite) scrolledComposite.getChildren()[0]).getChildren().length
+ );
+
+ if (numberOfItemsInScrolledComp > NUM_FOR_FILTER_POPUP)
+ {
+ swtBotShell.bot().text().setText(text);
+ // Scan only the list content to avoid unrelated labels.
+ found = syncExec(() -> {
+ Composite list = (Composite) scrolledComposite.getChildren()[0];
+ for (Widget w : list.getChildren()) {
+ if (w instanceof Label && text.equals(((Label) w).getText())) {
+ return true;
+ }
+ }
+ return false;
+ });
+ }
+ else
+ {
+ try {
+ swtBotShell.bot().label(text);
+ found = true;
+ } catch (WidgetNotFoundException e) {
+ found = false;
+ }
+ }
+ }
+ catch (Exception e)
+ {
+ e.printStackTrace();
+ found = false;
+ }
+ finally
+ {
+ try { swtBotShell.close(); } catch (Exception ignore) {}
+ }
+ return found;
+ }Optional: after setting the filter text, add a short wait for UI update (e.g., a DefaultCondition) to reduce flakiness.
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/operations/selectors/LaunchBarTargetSelector.java
around lines 129 to 174, ensure the popup shell is always closed and the filter
cleared by moving shell-close/cleanup into a finally block; after typing the
filter text wait briefly for the UI to update (e.g., a short DefaultCondition or
sync) before inspecting results; when the scrolled composite contains many
items, restrict the label scan to the scrolledComposite’s child controls (not
all shell labels) by retrieving its first child Composite and iterating that
child’s children only; in the short-list branch use bot().label(text) (or
SWTBotLabel lookup) instead of casting a generic Widget to Label; keep
WidgetNotFoundException handling but ensure the finally closes the popup and
clears the filter to avoid side effects.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/operations/selectors/LaunchBarTargetSelector.java (1)
138-183: Always close the popup; restrict scan scope; and wait after filtering to avoid flakiness.The popup remains open on early returns; scanning all shell labels risks false positives; and filtering lacks a stabilization wait. Close the shell in finally, clear the filter, and only inspect the ScrolledComposite’s content.
- click(); - - try - { - SWTBotShell swtBotShell = bot().shellWithId(LaunchBarWidgetIds.POPUP); - ScrolledComposite scrolledComposite = swtBotShell.bot().widget(widgetOfType(ScrolledComposite.class)); - int numberOfItemsInScrolledComp = syncExec(() -> - ((Composite) scrolledComposite.getChildren()[0]).getChildren().length - ); - // Scroll to the bottom if there are many items - if (numberOfItemsInScrolledComp > NUM_FOR_FILTER_POPUP) - { - scrollToBottom(swtBotShell.bot().widget(widgetOfType(ScrolledComposite.class))); - swtBotShell.bot().text().setText(text); - List<? extends Widget> labels = swtBotShell.bot().widgets(widgetOfType(Label.class)); - for (Widget widget : labels) - { - String labelText = syncExec(() -> ((Label) widget).getText()); - if (labelText.equals(text)) - { - return true; - } - } - return false; - } - else - { - Widget itemToCheck = swtBotShell.bot().widget(withText(text)); - String labelText = syncExec(() -> ((Label) itemToCheck).getText()); - return labelText.equals(text); - } - } - catch (WidgetNotFoundException e) - { - return false; - } - catch (Exception e) - { - e.printStackTrace(); - return false; - } + click(); + SWTBotShell swtBotShell = bot().shellWithId(LaunchBarWidgetIds.POPUP); + boolean found = false; + try + { + ScrolledComposite scrolledComposite = swtBotShell.bot().widget(widgetOfType(ScrolledComposite.class)); + int numberOfItemsInScrolledComp = syncExec(() -> { + org.eclipse.swt.widgets.Control content = scrolledComposite.getContent(); + return (content instanceof Composite) ? ((Composite) content).getChildren().length : 0; + }); + + if (numberOfItemsInScrolledComp > NUM_FOR_FILTER_POPUP) + { + // Filtering is sufficient; no need to scroll first. + swtBotShell.bot().text().setText(text); + // Wait for the list to update and the item to appear + swtBotShell.bot().waitUntil(new org.eclipse.swtbot.swt.finder.waits.DefaultCondition() { + @Override public boolean test() { + return syncExec(() -> { + org.eclipse.swt.widgets.Control content = scrolledComposite.getContent(); + if (content instanceof Composite) { + for (Widget w : ((Composite) content).getChildren()) { + if (w instanceof Label && text.equals(((Label) w).getText())) return true; + } + } + return false; + }); + } + @Override public String getFailureMessage() { return "Target not visible after filtering: " + text; } + }, 5000); + + found = syncExec(() -> { + org.eclipse.swt.widgets.Control content = scrolledComposite.getContent(); + if (content instanceof Composite) { + for (Widget w : ((Composite) content).getChildren()) { + if (w instanceof Label && text.equals(((Label) w).getText())) return true; + } + } + return false; + }); + } + else + { + try { + swtBotShell.bot().label(text); + found = true; + } catch (WidgetNotFoundException ignore) { + found = false; + } + } + } + catch (Exception e) + { + e.printStackTrace(); + found = false; + } + finally + { + try { swtBotShell.bot().text().setText(""); } catch (Exception ignore) {} + try { swtBotShell.close(); } catch (Exception ignore) {} + } + return found;Add import if missing:
import org.eclipse.swtbot.swt.finder.waits.DefaultCondition;
🧹 Nitpick comments (3)
tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/operations/selectors/LaunchBarTargetSelector.java (3)
20-35: Remove duplicate static imports and unused imports.
widgetOfTypeandwithTextare imported twice (Lines 8–10 and 33–34). Keep only one import each.ScrollBarandSWTBotLabelappear unused in this file.Apply:
-import org.eclipse.swt.widgets.ScrollBar; -import org.eclipse.swtbot.swt.finder.widgets.SWTBotLabel; -import static org.eclipse.swtbot.swt.finder.matchers.WidgetMatcherFactory.widgetOfType; -import static org.eclipse.swtbot.swt.finder.matchers.WidgetMatcherFactory.withText;
109-129: Constrain lookup to the list content and wait for UI filter to settle before selecting.
- Counting children via
getChildren()[0]is brittle. PrefergetContent()with type checks.- After typing in the filter, wait for the list to update; otherwise selection can race and flake.
- Constrain the label search to the ScrolledComposite’s content to avoid matching unrelated labels in the shell.
- ScrolledComposite scrolledComposite = swtBotShell.bot().widget(widgetOfType(ScrolledComposite.class)); - int numberOfItemsInScrolledComp = syncExec( - () -> ((Composite) scrolledComposite.getChildren()[0]).getChildren().length); + ScrolledComposite scrolledComposite = swtBotShell.bot().widget(widgetOfType(ScrolledComposite.class)); + int numberOfItemsInScrolledComp = syncExec(() -> { + org.eclipse.swt.widgets.Control content = scrolledComposite.getContent(); + return (content instanceof Composite) ? ((Composite) content).getChildren().length : 0; + }); @@ - swtBotShell.bot().text().setText(text); - itemToSelect = swtBotShell.bot().widget(allOf(widgetOfType(Label.class), withText(text))); + swtBotShell.bot().text().setText(text); + // wait until the filtered label becomes present inside the list + swtBotShell.bot().waitUntil(new org.eclipse.swtbot.swt.finder.waits.DefaultCondition() { + @Override public boolean test() { + return syncExec(() -> { + org.eclipse.swt.widgets.Control content = scrolledComposite.getContent(); + if (content instanceof Composite) { + for (Widget w : ((Composite) content).getChildren()) { + if (w instanceof Label && text.equals(((Label) w).getText())) return true; + } + } + return false; + }); + } + @Override public String getFailureMessage() { return "Filtered target not visible: " + text; } + }, 5000); + itemToSelect = syncExec(() -> { + org.eclipse.swt.widgets.Control content = scrolledComposite.getContent(); + if (content instanceof Composite) { + for (Widget w : ((Composite) content).getChildren()) { + if (w instanceof Label && text.equals(((Label) w).getText())) return (Label) w; + } + } + return null; + }); + if (itemToSelect == null) throw new WidgetNotFoundException("Target not found: " + text); @@ - itemToSelect = swtBotShell.bot().widget(allOf(widgetOfType(Label.class), withText(text))); + // Short list: still prefer constraining to list content + itemToSelect = syncExec(() -> { + org.eclipse.swt.widgets.Control content = scrolledComposite.getContent(); + if (content instanceof Composite) { + for (Widget w : ((Composite) content).getChildren()) { + if (w instanceof Label && text.equals(((Label) w).getText())) return (Label) w; + } + } + return null; + }); + if (itemToSelect == null) throw new WidgetNotFoundException("Target not found: " + text);Add import:
import org.eclipse.swtbot.swt.finder.waits.DefaultCondition;Verification: please run the test locally with an intentionally similar-named label present in the shell to ensure the constrained search avoids false positives.
131-136: Fix scrollToBottom: use content height instead of client area height.
setOrigin(0, clientArea.height)doesn’t guarantee bottom; compute y from content size minus client height.- syncExec(() -> { - scrolledComposite.setOrigin(0, scrolledComposite.getClientArea().height); - }); + syncExec(() -> { + org.eclipse.swt.widgets.Control content = scrolledComposite.getContent(); + if (content != null && !content.isDisposed()) { + int contentHeight = content.computeSize(SWT.DEFAULT, SWT.DEFAULT).y; + int clientHeight = scrolledComposite.getClientArea().height; + int y = Math.max(0, contentHeight - clientHeight); + scrolledComposite.setOrigin(0, y); + } + });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/executable/cases/project/IDFProjectLaunchTargetEditorFunctionalityTest.java(1 hunks)tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/operations/selectors/LaunchBarTargetSelector.java(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/executable/cases/project/IDFProjectLaunchTargetEditorFunctionalityTest.java
⏰ 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)
- GitHub Check: build
- GitHub Check: build_macos
- GitHub Check: build_windows
sigmaaa
left a comment
sigmaaa
left a comment
There was a problem hiding this comment.
LGTM! I suggested a small optimization, @alirana01 check if you agree. Thanks
...ssif/idf/ui/test/executable/cases/project/IDFProjectLaunchTargetEditorFunctionalityTest.java
Outdated
Show resolved
Hide resolved
...ssif/idf/ui/test/executable/cases/project/IDFProjectLaunchTargetEditorFunctionalityTest.java
Outdated
Show resolved
Hide resolved
| { | ||
| LaunchBarTargetSelector targetSelector = new LaunchBarTargetSelector(bot); | ||
| targetSelector.selectTarget("esp32c2"); | ||
| TestWidgetWaitUtility.waitForDialogToAppear(bot, "IDF Launch Target Changed", 20000); |
There was a problem hiding this comment.
I thought we weren’t able to find dialogs created with MessageDialog.openQuestion. Last time I checked in the 4.0 PR, I couldn’t find any dialogs created with this method. We should revisit this for the 4.0.0 part—maybe the difference lies in how we pass the shell to the dialog. For example, in the IDF Launch Target Changed dialog, we are using EclipseUtil.getShell().
There was a problem hiding this comment.
If its working on runner with green builds which I am not seeing we can use it and see what is causing this issue and fix that
....idf.ui.test/src/com/espressif/idf/ui/test/operations/selectors/LaunchBarTargetSelector.java
Outdated
Show resolved
Hide resolved
|
Please rebase and then do further changes and make sure the builds are green specially for tests related PRs |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/executable/cases/project/IDFProjectLaunchTargetEditorFunctionalityTest.java (3)
120-125: Prefer sync wait after build to reduce racesUsing async here can proceed before all follow-up jobs complete. Use the sync variant.
ProjectTestOperations.buildProjectUsingContextMenu(projectName, bot); ProjectTestOperations.waitForProjectBuild(bot); - TestWidgetWaitUtility.waitForOperationsInProgressToFinishAsync(bot); + TestWidgetWaitUtility.waitForOperationsInProgressToFinishSync(bot);
49-60: Rename @afterclass method for clarityMethod name suggests per-test cleanup. Rename for accuracy, no behavior change.
- public static void afterEachTest() + public static void afterTestClass()
35-38: Test ordering couples tests; ensure independence or document dependencyNAME_ASCENDING ties test outcomes together (e.g., deletion relies on prior creation). Consider making each test self‑sufficient or clearly documenting/order‑enforcing only where necessary.
Please confirm the suite won’t be split/parallelized by CI, as that can break order guarantees.
📜 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/IDFProjectLaunchTargetEditorFunctionalityTest.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/IDFProjectLaunchTargetEditorFunctionalityTest.java (4)
tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/common/WorkBenchSWTBot.java (1)
WorkBenchSWTBot(14-28)tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/operations/EnvSetupOperations.java (1)
EnvSetupOperations(12-85)tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/operations/ProjectTestOperations.java (1)
ProjectTestOperations(51-780)tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/operations/selectors/LaunchBarTargetSelector.java (1)
SuppressWarnings(42-185)
⏰ 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). (1)
- GitHub Check: build_windows
🔇 Additional comments (5)
tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/executable/cases/project/IDFProjectLaunchTargetEditorFunctionalityTest.java (5)
127-135: Stabilize “IDF Launch Target Changed” dialog handling: wait for close and jobsAdd waits after clicking “Yes” to avoid flakiness when the dialog is still closing or background work is running.
private static void whenChangeLaunchTarget() throws Exception { LaunchBarTargetSelector targetSelector = new LaunchBarTargetSelector(bot); targetSelector.selectTarget("esp32c2"); TestWidgetWaitUtility.waitForDialogToAppear(bot, "IDF Launch Target Changed", 20000); SWTBotShell shell = bot.shell("IDF Launch Target Changed"); shell.setFocus(); - bot.button("Yes").click(); + bot.button("Yes").click(); + bot.waitUntil(org.eclipse.swtbot.swt.finder.waits.Conditions.shellCloses(shell), 20000); + TestWidgetWaitUtility.waitForOperationsInProgressToFinishSync(bot); }If you prefer an import instead of the FQN, add:
+import org.eclipse.swtbot.swt.finder.waits.Conditions;
149-152: Also wait after deleting the launch targetEnsure the editor dialog is fully closed and workspace jobs settle before verification.
SWTBotShell shell = bot.shell("New ESP Target"); shell.setFocus(); - bot.button("Delete").click(); + bot.button("Delete").click(); + bot.waitUntil(org.eclipse.swtbot.swt.finder.waits.Conditions.shellCloses(shell), 20000); + TestWidgetWaitUtility.waitForOperationsInProgressToFinishSync(bot);
199-202: Wait for refresh-completed background workAdd a final sync wait after triggering Refresh to ensure the Project Explorer state is stable before assertions.
private static void whenRefreshProject() throws IOException { ProjectTestOperations.launchCommandUsingContextMenu(projectName, bot, "Refresh"); + TestWidgetWaitUtility.waitForOperationsInProgressToFinishSync(bot); }
167-190: Good: dialog presence checks and enabled-state wait improve stabilityValidating the “New Launch Target” dialog and waiting for “Next >” enablement reduces flakiness. The final sync wait after “Finish” is appropriate.
44-47: Fix wizard category typo to avoid test failure"EspressIf" prevents the New Project wizard node from being found.
- Fixture.givenNewEspressifIDFProjectIsSelected("EspressIf", "Espressif IDF Project"); + Fixture.givenNewEspressifIDFProjectIsSelected("Espressif", "Espressif IDF Project");
…get list. Add A/B/C markers to make sure tests run in correct(needed) order
AndriiFilippov
force-pushed
the
IEP-1629
branch
from
32a06ab to
b0777d5
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (7)
tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/operations/selectors/LaunchBarTargetSelector.java (2)
109-129: Stabilize selection after filtering; wait for the label to appear.After setting the filter text, immediately looking up the Label is race‑prone. Wait for the filtered label to be found before retrieving its location.
Apply this diff:
@@ - if (numberOfItemsInScrolledComp > NUM_FOR_FILTER_POPUP) - { - swtBotShell.bot().text().setText(text); - itemToSelect = swtBotShell.bot().widget(allOf(widgetOfType(Label.class), withText(text))); - } + if (numberOfItemsInScrolledComp > NUM_FOR_FILTER_POPUP) + { + swtBotShell.bot().text().setText(text); + // Wait for UI to update the filtered list + swtBotShell.bot().waitUntil( + org.eclipse.swtbot.swt.finder.waits.Conditions.widgetIsFound(allOf(widgetOfType(Label.class), withText(text))), + 5000 + ); + itemToSelect = swtBotShell.bot().widget(allOf(widgetOfType(Label.class), withText(text))); + }If you prefer a tighter scope, consider searching only within the ScrolledComposite’s content composite.
131-136: Fix scrollToBottom: current origin calculation won’t reach the bottom.Setting origin to clientArea.height doesn’t guarantee scrolling to the bottom. Use the vertical bar max (or content size) instead.
Apply this diff:
public void scrollToBottom(ScrolledComposite scrolledComposite) { syncExec(() -> { - scrolledComposite.setOrigin(0, scrolledComposite.getClientArea().height); + // Prefer using the vertical scrollbar if present + ScrollBar vBar = scrolledComposite.getVerticalBar(); + if (vBar != null) { + int max = vBar.getMaximum(); + vBar.setSelection(max); + scrolledComposite.setOrigin(0, max); + } else { + // Fallback: use content height if available + Composite content = (Composite) scrolledComposite.getChildren()[0]; + int y = content.getSize().y; + scrolledComposite.setOrigin(0, y); + } }); }tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/executable/cases/project/IDFProjectLaunchTargetEditorFunctionalityTest.java (5)
49-60: Rename method to reflect @afterclass lifecycle.Minor naming nit: method is annotated with @afterclass but named afterEachTest.
Apply this diff:
- @AfterClass - public static void afterEachTest() + @AfterClass + public static void afterTestClass()
12-21: Import Conditions used in waits.Required for the wait calls suggested below.
Apply this diff:
import org.eclipse.swtbot.swt.finder.junit.SWTBotJunit4ClassRunner; import org.eclipse.swtbot.swt.finder.widgets.SWTBotShell; +import org.eclipse.swtbot.swt.finder.waits.Conditions; import org.junit.AfterClass;
127-135: Wait for confirmation dialog to close and background work to settle.Prevents flakiness after changing the launch target.
Apply this diff:
TestWidgetWaitUtility.waitForDialogToAppear(bot, "IDF Launch Target Changed", 20000); SWTBotShell shell = bot.shell("IDF Launch Target Changed"); shell.setFocus(); - bot.button("Yes").click(); + bot.button("Yes").click(); + bot.waitUntil(Conditions.shellCloses(shell), 20000); + TestWidgetWaitUtility.waitForOperationsInProgressToFinishSync(bot);
143-152: After deleting, wait for dialog close and workspace to settle.Stabilizes subsequent checks for deletion.
Apply this diff:
SWTBotShell shell = bot.shell("New ESP Target"); shell.setFocus(); - bot.button("Delete").click(); + bot.button("Delete").click(); + bot.waitUntil(Conditions.shellCloses(shell), 20000); + TestWidgetWaitUtility.waitForOperationsInProgressToFinishSync(bot);
199-202: Wait for workspace refresh to finish.Ensures Project Explorer state is stable before assertions.
Apply this diff:
private static void whenRefreshProject() throws IOException { ProjectTestOperations.launchCommandUsingContextMenu(projectName, bot, "Refresh"); + TestWidgetWaitUtility.waitForOperationsInProgressToFinishSync(bot); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/executable/cases/project/IDFProjectLaunchTargetEditorFunctionalityTest.java(1 hunks)tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/operations/ProjectTestOperations.java(1 hunks)tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/operations/selectors/LaunchBarTargetSelector.java(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/operations/ProjectTestOperations.java
🧰 Additional context used
🧬 Code graph analysis (1)
tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/executable/cases/project/IDFProjectLaunchTargetEditorFunctionalityTest.java (4)
tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/common/WorkBenchSWTBot.java (1)
WorkBenchSWTBot(14-28)tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/operations/EnvSetupOperations.java (1)
EnvSetupOperations(12-85)tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/operations/ProjectTestOperations.java (1)
ProjectTestOperations(51-789)tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/operations/selectors/LaunchBarTargetSelector.java (1)
SuppressWarnings(42-185)
⏰ 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_macos
- GitHub Check: build_windows
🔇 Additional comments (4)
tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/operations/selectors/LaunchBarTargetSelector.java (1)
138-184: Always close the popup; restrict label scan to list content; add small wait after filtering.Prevents side effects/flakiness; avoids matching unrelated shell labels; improves reliability after typing filter text.
Apply this diff:
public boolean isTargetPresent(String text) { click(); - try - { - SWTBotShell swtBotShell = bot().shellWithId(LaunchBarWidgetIds.POPUP); - ScrolledComposite scrolledComposite = swtBotShell.bot().widget(widgetOfType(ScrolledComposite.class)); - - int numberOfItemsInScrolledComp = syncExec(() -> - ((Composite) scrolledComposite.getChildren()[0]).getChildren().length - ); - - // Scroll to the bottom if there are many items - if (numberOfItemsInScrolledComp > NUM_FOR_FILTER_POPUP) - { - scrollToBottom(swtBotShell.bot().widget(widgetOfType(ScrolledComposite.class))); - swtBotShell.bot().text().setText(text); - - List<? extends Widget> labels = swtBotShell.bot().widgets(widgetOfType(Label.class)); - for (Widget widget : labels) - { - String labelText = syncExec(() -> ((Label) widget).getText()); - if (labelText.equals(text)) - { - return true; - } - } - return false; - } - else - { - Widget itemToCheck = swtBotShell.bot().widget(withText(text)); - String labelText = syncExec(() -> ((Label) itemToCheck).getText()); - return labelText.equals(text); - } - } - catch (WidgetNotFoundException e) - { - return false; - } - catch (Exception e) - { - e.printStackTrace(); - return false; - } + SWTBotShell swtBotShell = bot().shellWithId(LaunchBarWidgetIds.POPUP); + boolean found = false; + try { + ScrolledComposite scrolledComposite = swtBotShell.bot().widget(widgetOfType(ScrolledComposite.class)); + int numberOfItemsInScrolledComp = syncExec(() -> + ((Composite) scrolledComposite.getChildren()[0]).getChildren().length + ); + + if (numberOfItemsInScrolledComp > NUM_FOR_FILTER_POPUP) { + scrollToBottom(scrolledComposite); + swtBotShell.bot().text().setText(text); + // Wait briefly for filter to apply + swtBotShell.bot().waitUntil( + org.eclipse.swtbot.swt.finder.waits.Conditions.widgetIsFound(allOf(widgetOfType(Label.class), withText(text))), + 2000 + ); + // Restrict scan to the list content + found = syncExec(() -> { + Composite list = (Composite) scrolledComposite.getChildren()[0]; + for (Widget w : list.getChildren()) { + if (w instanceof Label && text.equals(((Label) w).getText())) { + return true; + } + } + return false; + }); + } else { + try { + swtBotShell.bot().label(text); + found = true; + } catch (WidgetNotFoundException ignore) { + found = false; + } + } + } catch (Exception e) { + e.printStackTrace(); + found = false; + } finally { + try { swtBotShell.close(); } catch (Exception ignore) {} + } + return found; }Note: since Conditions is referenced, ensure the class import exists:
import org.eclipse.swtbot.swt.finder.waits.Conditions;tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/executable/cases/project/IDFProjectLaunchTargetEditorFunctionalityTest.java (3)
160-165: LGTM: clear verification of deletion.Asserting on isTargetPresent keeps the test outcome explicit.
175-183: LGTM: robust step to Next with enablement wait.Good use of widgetIsEnabled before clicking Next.
43-47: Fix wizard category string: “EspressIf” → “Espressif”.Incorrect capitalization will prevent finding the wizard node and fail setup.
Apply this diff:
- Fixture.givenNewEspressifIDFProjectIsSelected("EspressIf", "Espressif IDF Project"); + Fixture.givenNewEspressifIDFProjectIsSelected("Espressif", "Espressif IDF Project");
....idf.ui.test/src/com/espressif/idf/ui/test/operations/selectors/LaunchBarTargetSelector.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/operations/selectors/LaunchBarTargetSelector.java (3)
105-125: Prefer ScrolledComposite#getContent() over children[0]; add a tiny wait after filtering.More robust and less brittle than indexing the first child; short sleep helps UI settle post-filter.
- SWTBotShell swtBotShell = bot().shellWithId(LaunchBarWidgetIds.POPUP); - ScrolledComposite scrolledComposite = swtBotShell.bot().widget(widgetOfType(ScrolledComposite.class)); - int numberOfItemsInScrolledComp = syncExec( - () -> ((Composite) scrolledComposite.getChildren()[0]).getChildren().length); - Label itemToSelect; + SWTBotShell swtBotShell = bot().shellWithId(LaunchBarWidgetIds.POPUP); + ScrolledComposite scrolledComposite = swtBotShell.bot().widget(widgetOfType(ScrolledComposite.class)); + Composite list = syncExec(() -> (Composite) scrolledComposite.getContent()); + int numberOfItemsInScrolledComp = syncExec(() -> list.getChildren().length); + Label itemToSelect; @@ - swtBotShell.bot().text().setText(text); + swtBotShell.bot().text().setText(text); + // Give the UI a moment to apply the filter to the list. + swtBotShell.bot().sleep(150);
127-132: Fix scrolling: current origin uses client height, not bottom of content.Compute content height and scroll to max visible origin.
- syncExec(() -> { - scrolledComposite.setOrigin(0, scrolledComposite.getClientArea().height); - }); + syncExec(() -> { + org.eclipse.swt.widgets.Control content = scrolledComposite.getContent(); + if (content != null && !content.isDisposed()) { + org.eclipse.swt.graphics.Point size = content.computeSize(SWT.DEFAULT, SWT.DEFAULT); + int y = Math.max(0, size.y - scrolledComposite.getClientArea().height); + scrolledComposite.setOrigin(0, y); + } + });
134-180: Always close the popup; scope search to list content; avoid broad label scans.Prevents flakiness and false positives; uses shell.close() in finally and limits search to the ScrolledComposite content.
- public boolean isTargetPresent(String text) - { - click(); - - try - { - SWTBotShell swtBotShell = bot().shellWithId(LaunchBarWidgetIds.POPUP); - ScrolledComposite scrolledComposite = swtBotShell.bot().widget(widgetOfType(ScrolledComposite.class)); - - int numberOfItemsInScrolledComp = syncExec(() -> - ((Composite) scrolledComposite.getChildren()[0]).getChildren().length - ); - - // Scroll to the bottom if there are many items - if (numberOfItemsInScrolledComp > NUM_FOR_FILTER_POPUP) - { - scrollToBottom(swtBotShell.bot().widget(widgetOfType(ScrolledComposite.class))); - swtBotShell.bot().text().setText(text); - - List<? extends Widget> labels = swtBotShell.bot().widgets(widgetOfType(Label.class)); - for (Widget widget : labels) - { - String labelText = syncExec(() -> ((Label) widget).getText()); - if (labelText.equals(text)) - { - return true; - } - } - return false; - } - else - { - Widget itemToCheck = swtBotShell.bot().widget(withText(text)); - String labelText = syncExec(() -> ((Label) itemToCheck).getText()); - return labelText.equals(text); - } - } - catch (WidgetNotFoundException e) - { - return false; - } - catch (Exception e) - { - e.printStackTrace(); - return false; - } - } + public boolean isTargetPresent(String text) + { + click(); + SWTBotShell swtBotShell = bot().shellWithId(LaunchBarWidgetIds.POPUP); + boolean found = false; + try + { + ScrolledComposite scrolledComposite = swtBotShell.bot().widget(widgetOfType(ScrolledComposite.class)); + Composite list = syncExec(() -> (Composite) scrolledComposite.getContent()); + int numberOfItemsInScrolledComp = syncExec(() -> list.getChildren().length); + + if (numberOfItemsInScrolledComp > NUM_FOR_FILTER_POPUP) + { + swtBotShell.bot().text().setText(text); + // Give the UI a moment to apply the filter to the list. + swtBotShell.bot().sleep(150); + + found = syncExec(() -> { + for (Widget w : list.getChildren()) { + if (w instanceof Label && text.equals(((Label) w).getText())) { + return true; + } + } + return false; + }); + } + else + { + try { + swtBotShell.bot().label(text); + found = true; + } catch (WidgetNotFoundException e) { + found = false; + } + } + } + catch (Exception e) + { + found = false; + } + finally + { + try { swtBotShell.close(); } catch (Exception ignore) {} + } + return found; + }
📜 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/operations/selectors/LaunchBarTargetSelector.java(3 hunks)
⏰ 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)
- GitHub Check: build_windows
- GitHub Check: build_macos
- GitHub Check: build
🔇 Additional comments (3)
tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/operations/selectors/LaunchBarTargetSelector.java (3)
11-12: LGTM on adding List import.
28-28: No action needed.
30-30: LGTM on allOf import.
Launch Target Editor test cases:
Description
Please include a summary of the change and which issue is fixed.
Fixes # (IEP-1629)
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