WIP: SWTBot: add additional target support for the Build & Flash test case.#1401
WIP: SWTBot: add additional target support for the Build & Flash test case.#1401AndriiFilippov wants to merge 8 commits intomasterfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughRefactors the flash test to iterate multiple (target,port) pairs with helpers for target switching and port selection; hardens LaunchBarTargetSelector with scrolling and filter-popup handling; and bumps the Linux CI ESP-IDF action version from v5.4 to v6.0. Changes
Sequence DiagramsequenceDiagram
participant TestRunner as Test Runner
participant Selector as LaunchBarTargetSelector
participant Builder as Build System
participant PortSelector as Serial Port Selector
participant Flasher as Flash Operation
participant Verifier as Verification
loop For each (target, port) in TARGETS
TestRunner->>Selector: whenChangeLaunchTarget(target, skipDialogOnThisRun?)
Selector-->>TestRunner: dialog handled / target switched
TestRunner->>Builder: build project
Builder-->>TestRunner: build complete
TestRunner->>PortSelector: whenSelectLaunchTargetSerialPort(portPrefixOrExact)
alt exact match found
PortSelector-->>TestRunner: exact port selected
else prefix match found
PortSelector-->>TestRunner: first matching port selected
else no match
PortSelector-->>TestRunner: AssertionError (no match)
end
TestRunner->>Flasher: flash
Flasher-->>TestRunner: flash complete
TestRunner->>Verifier: verify & wait for in-progress UI ops
Verifier-->>TestRunner: verification complete
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/operations/selectors/LaunchBarTargetSelector.java (2)
148-149: Redundant re-fetch ofScrolledComposite.Line 149 re-queries
widgetOfType(ScrolledComposite.class)when thescrolledCompositevariable from Line 142 is already in scope.Fix
- scrollToBottom(swtBotShell.bot().widget(widgetOfType(ScrolledComposite.class))); + scrollToBottom(scrolledComposite);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/operations/selectors/LaunchBarTargetSelector.java` around lines 148 - 149, The code redundantly re-fetches the ScrolledComposite when calling scrollToBottom; instead of calling swtBotShell.bot().widget(widgetOfType(ScrolledComposite.class)) inside the if in LaunchBarTargetSelector, pass the existing scrolledComposite local variable (declared earlier) into scrollToBottom so scrollToBottom(scrolledComposite) is used and avoid the extra widget lookup.
93-129: Misleading comment and redundant pre-scroll in the<= NUM_FOR_FILTER_POPUPbranch.
The comment on Lines 101–102 says "If popup is 'long' (no filter)", but the condition
<= NUM_FOR_FILTER_POPUPmatches the short / no-filter popup. The comment should reflect that.The
scrollToBottomat Line 104 is redundant with the retry scroll at Line 119—for the<= 7branch, both paths scroll to bottom. The pre-scroll is either always sufficient (making the catch dead code) or insufficient (making it wasted work). Consider keeping only the try/catch-based scroll for clarity.Suggested simplification
- // If popup is "long" (no filter) we may need to scroll to reach items near the - // bottom. - if (numberOfItemsInScrolledComp <= NUM_FOR_FILTER_POPUP) { - scrollToBottom(scrolledComposite); - } - Label itemToSelect; if (numberOfItemsInScrolledComp > NUM_FOR_FILTER_POPUP) { // Filter pop-up: typing should bring the item into view, no need to scroll. swtBotShell.bot().text().setText(text); itemToSelect = swtBotShell.bot().widget(allOf(widgetOfType(Label.class), withText(text))); } else { - // Non-filter pop-up: try to find it; if not found (still off-screen), scroll - // and retry. + // Non-filter (short) pop-up: item may be off-screen, scroll and retry if needed. try { itemToSelect = swtBotShell.bot().widget(allOf(widgetOfType(Label.class), withText(text))); } catch (Exception firstTryFailed) { scrollToBottom(scrolledComposite); itemToSelect = swtBotShell.bot().widget(allOf(widgetOfType(Label.class), withText(text))); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/operations/selectors/LaunchBarTargetSelector.java` around lines 93 - 129, The comment in selectTarget is inverted and the preemptive scrollToBottom call before checking numberOfItemsInScrolledComp is redundant; update the comment to correctly describe the branch (<= NUM_FOR_FILTER_POPUP is the short/non-filter popup) and remove the initial scrollToBottom at the top of selectTarget so only the retry-based scroll in the else (catch) branch using scrollToBottom remains; keep the try/catch lookup that scrolls on failure to ensure we only scroll when the item is not found.tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/executable/cases/project/NewEspressifIDFProjectFlashProcessTest.java (2)
72-80: Hardcoded device path will break on other machines / CI agents.
"/dev/ttyUSB1 Dual RS232-HS"is specific to one Linux host. Consider reading ports from environment variables or system properties (e.g.,System.getenv("ESP32_PORT")) so CI and other developers can override them without code changes. The same applies to the commented-out targets once they are enabled.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/executable/cases/project/NewEspressifIDFProjectFlashProcessTest.java` around lines 72 - 80, The TARGETS array in NewEspressifIDFProjectFlashProcessTest currently contains a hardcoded device path (" /dev/ttyUSB1 Dual RS232-HS") which will fail on other machines; update the TargetPort population (the TARGETS constant in class NewEspressifIDFProjectFlashProcessTest) to read port values from an environment variable or system property (e.g., System.getenv("ESP32_PORT") or System.getProperty("esp32.port")) with a sensible default or skip if unset, and apply the same pattern for any future/commented TargetPort entries so CI and other developers can override ports without editing code.
54-63: PreferAssume.assumeTrueoverassertTrue(true)for platform gating.Using
assertTrue(true)silently marks the test as "passed" on non-Linux platforms, hiding the fact that it was never exercised.Assume.assumeTrue(SystemUtils.IS_OS_LINUX)at the top of the method will correctly report the test as skipped, giving better visibility in CI dashboards.♻️ Proposed refactor
public void givenNewProjectCreatedBuiltWhenSelectSerialPortWhenFlashThenCheckFlashedSuccessfully() throws Exception { - if (SystemUtils.IS_OS_LINUX) // temporary solution until new ESP boards arrive for Windows - { + org.junit.Assume.assumeTrue("Skipping: only runs on Linux until new ESP boards arrive for Windows", + SystemUtils.IS_OS_LINUX); + { Fixture.givenNewEspressifIDFProjectIsSelected("EspressIf", "Espressif IDF Project"); Fixture.givenProjectNameIs("NewProjectFlashTest"); Fixture.whenNewProjectIsSelected(); Fixture.whenTurnOffOpenSerialMonitorAfterFlashingInLaunchConfig(); Fixture.whenBuildAndFlashForAllTargetsSequentially(); - } else { - assertTrue(true); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/executable/cases/project/NewEspressifIDFProjectFlashProcessTest.java` around lines 54 - 63, The test currently gates Linux-only behavior with an if/else and uses assertTrue(true) in NewEspressifIDFProjectFlashProcessTest which hides skipped tests; replace the if/else gating by adding an explicit assumption at the start of the test method using Assume.assumeTrue(SystemUtils.IS_OS_LINUX) (import org.junit.Assume) and then move the Linux-only block (Fixture.givenNewEspressifIDFProjectIsSelected, Fixture.givenProjectNameIs, Fixture.whenNewProjectIsSelected, Fixture.whenTurnOffOpenSerialMonitorAfterFlashingInLaunchConfig, Fixture.whenBuildAndFlashForAllTargetsSequentially) under that assumption so non-Linux runs are reported as skipped instead of passing.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/operations/selectors/LaunchBarTargetSelector.java`:
- Around line 137-171: isTargetPresent opens the launch-bar popup with click()
but never closes it, causing leftover UI state; wrap the popup inspection logic
in a try/finally and in the finally block dismiss the popup (use the obtained
SWTBotShell swtBotShell to pressEscape() or call close/pressEscape via bot()) so
every return path (both the scrolledComposite branch and the direct widget
branch, and on exceptions) will always close the popup before returning.
- Around line 160-163: The code in LaunchBarTargetSelector is performing an
unsafe cast by calling swtBotShell.bot().widget(withText(text)) and then casting
to Label, and it redundantly re-checks labelText.equals(text); instead, locate
the widget as a Label by using
swtBotShell.bot().widget(allOf(widgetOfType(Label.class), withText(text))) so
the cast to Label is safe, then you can simply return true if the widget is
found (no extra equals check), using syncExec only if you need to read the label
text; replace the current widget(withText(...)) + cast + equals logic with this
pattern (referencing widgetOfType(Label.class), withText, allOf,
swtBotShell.bot().widget and syncExec).
---
Nitpick comments:
In
`@tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/executable/cases/project/NewEspressifIDFProjectFlashProcessTest.java`:
- Around line 72-80: The TARGETS array in NewEspressifIDFProjectFlashProcessTest
currently contains a hardcoded device path (" /dev/ttyUSB1 Dual RS232-HS") which
will fail on other machines; update the TargetPort population (the TARGETS
constant in class NewEspressifIDFProjectFlashProcessTest) to read port values
from an environment variable or system property (e.g.,
System.getenv("ESP32_PORT") or System.getProperty("esp32.port")) with a sensible
default or skip if unset, and apply the same pattern for any future/commented
TargetPort entries so CI and other developers can override ports without editing
code.
- Around line 54-63: The test currently gates Linux-only behavior with an
if/else and uses assertTrue(true) in NewEspressifIDFProjectFlashProcessTest
which hides skipped tests; replace the if/else gating by adding an explicit
assumption at the start of the test method using
Assume.assumeTrue(SystemUtils.IS_OS_LINUX) (import org.junit.Assume) and then
move the Linux-only block (Fixture.givenNewEspressifIDFProjectIsSelected,
Fixture.givenProjectNameIs, Fixture.whenNewProjectIsSelected,
Fixture.whenTurnOffOpenSerialMonitorAfterFlashingInLaunchConfig,
Fixture.whenBuildAndFlashForAllTargetsSequentially) under that assumption so
non-Linux runs are reported as skipped instead of passing.
In
`@tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/operations/selectors/LaunchBarTargetSelector.java`:
- Around line 148-149: The code redundantly re-fetches the ScrolledComposite
when calling scrollToBottom; instead of calling
swtBotShell.bot().widget(widgetOfType(ScrolledComposite.class)) inside the if in
LaunchBarTargetSelector, pass the existing scrolledComposite local variable
(declared earlier) into scrollToBottom so scrollToBottom(scrolledComposite) is
used and avoid the extra widget lookup.
- Around line 93-129: The comment in selectTarget is inverted and the preemptive
scrollToBottom call before checking numberOfItemsInScrolledComp is redundant;
update the comment to correctly describe the branch (<= NUM_FOR_FILTER_POPUP is
the short/non-filter popup) and remove the initial scrollToBottom at the top of
selectTarget so only the retry-based scroll in the else (catch) branch using
scrollToBottom remains; keep the try/catch lookup that scrolls on failure to
ensure we only scroll when the item is not found.
| 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 Label> labels = swtBotShell.bot().widgets(widgetOfType(Label.class)); | ||
| for (Label label : labels) { | ||
| String labelText = syncExec(label::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.
Popup is never dismissed after the presence check.
click() on Line 138 opens the target popup, but none of the return paths close it. Unlike selectTarget (where clicking an item implicitly closes the popup), isTargetPresent only inspects—leaving the popup open for subsequent test operations, which can cause flakiness.
Consider pressing Escape or clicking elsewhere to dismiss the popup before returning.
Sketch: dismiss popup in a finally block
public boolean isTargetPresent(String text) {
click();
try {
SWTBotShell swtBotShell = bot().shellWithId(LaunchBarWidgetIds.POPUP);
+ try {
ScrolledComposite scrolledComposite = swtBotShell.bot().widget(widgetOfType(ScrolledComposite.class));
// ... existing logic ...
+ } finally {
+ // Dismiss the popup so subsequent operations are not affected
+ swtBotShell.close();
+ }
} catch (WidgetNotFoundException e) {
return false;
} catch (Exception e) {
e.printStackTrace();
return false;
}
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/operations/selectors/LaunchBarTargetSelector.java`
around lines 137 - 171, isTargetPresent opens the launch-bar popup with click()
but never closes it, causing leftover UI state; wrap the popup inspection logic
in a try/finally and in the finally block dismiss the popup (use the obtained
SWTBotShell swtBotShell to pressEscape() or call close/pressEscape via bot()) so
every return path (both the scrolledComposite branch and the direct widget
branch, and on exceptions) will always close the popup before returning.
| } else { | ||
| Widget itemToCheck = swtBotShell.bot().widget(withText(text)); | ||
| String labelText = syncExec(() -> ((Label) itemToCheck).getText()); | ||
| return labelText.equals(text); |
There was a problem hiding this comment.
Unsafe cast to Label and redundant text re-check.
Line 161 uses withText(text) without constraining to Label.class, so a non-Label widget (e.g., Text) could match—causing a ClassCastException on Line 162. The > NUM_FOR_FILTER_POPUP branch (Line 152) correctly uses widgetOfType(Label.class), and selectTarget uses allOf(widgetOfType(Label.class), withText(text)). Use the same pattern here.
Additionally, the labelText.equals(text) check on Line 163 is always true since the widget was already matched by withText(text).
Proposed fix
} else {
- Widget itemToCheck = swtBotShell.bot().widget(withText(text));
- String labelText = syncExec(() -> ((Label) itemToCheck).getText());
- return labelText.equals(text);
+ swtBotShell.bot().widget(allOf(widgetOfType(Label.class), withText(text)));
+ return true;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/operations/selectors/LaunchBarTargetSelector.java`
around lines 160 - 163, The code in LaunchBarTargetSelector is performing an
unsafe cast by calling swtBotShell.bot().widget(withText(text)) and then casting
to Label, and it redundantly re-checks labelText.equals(text); instead, locate
the widget as a Label by using
swtBotShell.bot().widget(allOf(widgetOfType(Label.class), withText(text))) so
the cast to Label is safe, then you can simply return true if the widget is
found (no extra equals check), using syncExec only if you need to read the label
text; replace the current widget(withText(...)) + cast + equals logic with this
pattern (referencing widgetOfType(Label.class), withText, allOf,
swtBotShell.bot().widget and syncExec).
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/executable/cases/project/NewEspressifIDFProjectFlashProcessTest.java (2)
135-143: Duplicate ofwhenChangeLaunchTarget— delegate instead.The body of
whenChangeLaunchTargetBackToESP32()is identical towhenChangeLaunchTarget("esp32", false). Any future fix to the dialog handling (e.g. button label change, focus timing) would have to be made in two places. Also, the indentation here is one level deeper than the surrounding methods.♻️ Proposed refactor
- private static void whenChangeLaunchTargetBackToESP32() throws Exception { - LaunchBarTargetSelector targetSelector = new LaunchBarTargetSelector(bot); - targetSelector.selectTarget("esp32"); - TestWidgetWaitUtility.waitForDialogToAppear(bot, "IDF Launch Target Changed", 20000); - SWTBotShell shell = bot.shell("IDF Launch Target Changed"); - shell.setFocus(); - bot.button("Yes").click(); - TestWidgetWaitUtility.waitForOperationsInProgressToFinishAsync(bot); - } + private static void whenChangeLaunchTargetBackToESP32() throws Exception { + whenChangeLaunchTarget("esp32", false); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/executable/cases/project/NewEspressifIDFProjectFlashProcessTest.java` around lines 135 - 143, The method whenChangeLaunchTargetBackToESP32() duplicates whenChangeLaunchTarget("esp32", false); replace its body with a single delegation to whenChangeLaunchTarget("esp32", false) (and fix the extra indentation) so dialog handling is centralized; locate the method and update its implementation to call whenChangeLaunchTarget rather than repeating the LaunchBarTargetSelector/selectTarget, TestWidgetWaitUtility, SWTBotShell focus, and button click logic.
216-224:TargetPortcould be arecord.This module requires JavaSE-21 (per MANIFEST.MF), which fully supports records.
TargetPortis a textbook candidate for conversion, eliminating boilerplate and ensuring immutability:private record TargetPort(String target, String port) {}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/executable/cases/project/NewEspressifIDFProjectFlashProcessTest.java` around lines 216 - 224, Replace the private static class TargetPort with a Java record to eliminate boilerplate and enforce immutability: remove the explicit fields and constructor and declare a private static record TargetPort(String target, String port) so callers use TargetPort.target() and TargetPort.port() (or keep existing field-access usage by adjusting references to the record accessors).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/executable/cases/project/NewEspressifIDFProjectFlashProcessTest.java`:
- Around line 104-119: The loop in whenBuildAndFlashForAllTargetsSequentially
implicitly assumes TARGETS[0].target is the post-creation target by using
skipTargetChangeDialog = (i == 0); change this to explicitly check the actual
selected launch target (compare tp.target against the current selected target
via the same API the test uses to read selection) or assert
TARGETS[0].target.equals("esp32") at the start of
whenBuildAndFlashForAllTargetsSequentially to make the precondition explicit;
additionally, ensure cleanup always runs by wrapping the loop caller so that
whenChangeLaunchTargetBackToESP32() is invoked in a finally block (or add a
try/finally around the loop inside whenBuildAndFlashForAllTargetsSequentially)
to restore the launch target even if an iteration throws.
- Around line 73-82: The TARGETS array in NewEspressifIDFProjectFlashProcessTest
is brittle due to hardcoded /dev/ttyUSBN paths and chip descriptions; change it
to load target-to-port mappings from a configurable source (environment
variable, system property, or external config file/JSON) instead of the static
TARGETS initializer, and update any code that references TARGETS (e.g., tests
that call into the flash selection logic) to read that configuration at test
startup; alternatively, support resolving ports via stable identifiers (udev
symlink names or serial numbers) by adding a resolver that maps logical target
names to /dev entries, and document expected CI configuration in the test
README.
---
Nitpick comments:
In
`@tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/executable/cases/project/NewEspressifIDFProjectFlashProcessTest.java`:
- Around line 135-143: The method whenChangeLaunchTargetBackToESP32() duplicates
whenChangeLaunchTarget("esp32", false); replace its body with a single
delegation to whenChangeLaunchTarget("esp32", false) (and fix the extra
indentation) so dialog handling is centralized; locate the method and update its
implementation to call whenChangeLaunchTarget rather than repeating the
LaunchBarTargetSelector/selectTarget, TestWidgetWaitUtility, SWTBotShell focus,
and button click logic.
- Around line 216-224: Replace the private static class TargetPort with a Java
record to eliminate boilerplate and enforce immutability: remove the explicit
fields and constructor and declare a private static record TargetPort(String
target, String port) so callers use TargetPort.target() and TargetPort.port()
(or keep existing field-access usage by adjusting references to the record
accessors).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ffac9126-0b03-4a9d-a51a-9fefd911347b
📒 Files selected for processing (1)
tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/executable/cases/project/NewEspressifIDFProjectFlashProcessTest.java
| private static final TargetPort[] TARGETS = new TargetPort[] { | ||
| new TargetPort("esp32", "/dev/ttyUSB1 Dual RS232-HS"), | ||
| new TargetPort("esp32c61", "/dev/ttyUSB0 CP2102N USB to UART Bridge Controller"), | ||
| new TargetPort("esp32c5", "/dev/ttyUSB2 CP2102N USB to UART Bridge Controller"), | ||
| new TargetPort("esp32h2", "/dev/ttyUSB3 CP2102N USB to UART Bridge Controller"), | ||
| new TargetPort("esp32h4", "/dev/ttyUSB4 CP2102N USB to UART Bridge Controller"), | ||
| new TargetPort("esp32s2", "/dev/ttyUSB5 CP2102N USB to UART Bridge Controller"), | ||
| new TargetPort("esp32s3", "/dev/ttyUSB6 CP2102N USB to UART Bridge Controller"), | ||
| new TargetPort("esp32c3", "/dev/ttyUSB7 CP2102N USB to UART Bridge Controller") | ||
| }; |
There was a problem hiding this comment.
Hardcoded ports/descriptors make this test brittle and environment-specific.
The TARGETS array pins each target to an exact /dev/ttyUSBN device path and a specific USB-UART chip description (e.g. Dual RS232-HS, CP2102N USB to UART Bridge Controller). This assumes:
- A fixed enumeration order of USB devices on the CI runner (kernel assigns
ttyUSBNin the order devices are probed, which is not stable across reboots, replugging, or hub changes). - A specific collection of adapters is always attached (different boards ship with different USB bridges — e.g. some ESP32-S3/C5 dev kits use native USB, not CP2102N).
If any device re-enumerates, the test will silently pick the wrong board and flash it, or fall back to the startsWith match on line 164 and still flash the wrong target. At minimum, consider:
- Reading the target/port map from a configuration file or environment variable so the CI maintainer can adjust without a code change.
- Using udev symlinks (e.g.
/dev/esp32s3) pinned by USB serial number rather thanttyUSBN. - Documenting the exact runner hardware layout in a README alongside this test.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/executable/cases/project/NewEspressifIDFProjectFlashProcessTest.java`
around lines 73 - 82, The TARGETS array in
NewEspressifIDFProjectFlashProcessTest is brittle due to hardcoded /dev/ttyUSBN
paths and chip descriptions; change it to load target-to-port mappings from a
configurable source (environment variable, system property, or external config
file/JSON) instead of the static TARGETS initializer, and update any code that
references TARGETS (e.g., tests that call into the flash selection logic) to
read that configuration at test startup; alternatively, support resolving ports
via stable identifiers (udev symlink names or serial numbers) by adding a
resolver that maps logical target names to /dev entries, and document expected
CI configuration in the test README.
| private static void whenBuildAndFlashForAllTargetsSequentially() throws Exception { | ||
| for (int i = 0; i < TARGETS.length; i++) { | ||
| TargetPort tp = TARGETS[i]; | ||
|
|
||
| boolean skipTargetChangeDialog = (i == 0); | ||
| whenChangeLaunchTarget(tp.target, skipTargetChangeDialog); | ||
|
|
||
| whenProjectIsBuiltUsingContextMenu(); | ||
| whenSelectLaunchTargetSerialPort(tp.port); | ||
| whenFlashProject(); | ||
| thenVerifyFlashDoneSuccessfully(); | ||
|
|
||
| TestWidgetWaitUtility.waitForOperationsInProgressToFinishAsync(bot); | ||
| bot.sleep(500); | ||
| } | ||
| } |
There was a problem hiding this comment.
Implicit coupling between TARGETS[0] and the post-creation target.
skipTargetChangeDialog = (i == 0) silently assumes TARGETS[0].target equals the target the project was just created with (esp32). If someone later reorders TARGETS or adds a new entry at the top, the first iteration will skip the target-change dialog handling while actually needing it, and the test will hang on an unhandled "IDF Launch Target Changed" dialog.
Consider making the precondition explicit, for example by asserting TARGETS[0].target.equals("esp32") at the top of the method, or by comparing tp.target against the currently-selected target rather than using the loop index.
Additionally, if any iteration throws, the remaining targets are skipped and the launch target is left on a non-esp32 value (the whenChangeLaunchTargetBackToESP32() call on line 61 won't run). Wrapping the loop/restore in a try/finally in the test method would make the cleanup reliable for local runs.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/executable/cases/project/NewEspressifIDFProjectFlashProcessTest.java`
around lines 104 - 119, The loop in whenBuildAndFlashForAllTargetsSequentially
implicitly assumes TARGETS[0].target is the post-creation target by using
skipTargetChangeDialog = (i == 0); change this to explicitly check the actual
selected launch target (compare tp.target against the current selected target
via the same API the test uses to read selection) or assert
TARGETS[0].target.equals("esp32") at the start of
whenBuildAndFlashForAllTargetsSequentially to make the precondition explicit;
additionally, ensure cleanup always runs by wrapping the loop caller so that
whenChangeLaunchTargetBackToESP32() is invoked in a finally block (or add a
try/finally around the loop inside whenBuildAndFlashForAllTargetsSequentially)
to restore the launch target even if an iteration throws.
| private static void whenSelectLaunchTargetSerialPort(String portPrefixOrExact) throws Exception { | ||
| LaunchBarTargetSelector targetSelector = new LaunchBarTargetSelector(bot); | ||
| targetSelector.clickEdit(); | ||
|
|
||
| TestWidgetWaitUtility.waitForDialogToAppear(bot, "New ESP Target", 20000); | ||
| SWTBotShell shell = bot.shell("New ESP Target"); | ||
| bot.comboBoxWithLabel("Serial Port:").setSelection("/dev/ttyUSB1 Dual RS232-HS"); | ||
| try { | ||
| bot.comboBoxWithLabel("Serial Port:").setSelection(portPrefixOrExact); | ||
| } catch (Exception ignored) { | ||
| String[] items = bot.comboBoxWithLabel("Serial Port:").items(); | ||
| String match = null; | ||
|
|
||
| for (String item : items) { | ||
| if (item != null && item.startsWith(portPrefixOrExact)) { | ||
| match = item; | ||
| break; | ||
| } | ||
| } | ||
|
|
||
| if (match == null) { | ||
| throw new AssertionError("No serial port matched: " + portPrefixOrExact + " ; available=" | ||
| + String.join(", ", items)); | ||
| } | ||
|
|
||
| bot.comboBoxWithLabel("Serial Port:").setSelection(match); | ||
| } | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Identify which exception type setSelection throws when the item is absent,
# to help narrow the catch clause.
rg -nP 'setSelection\s*\(' -C2 --type=java | head -80
rg -nP 'class\s+SWTBotCombo\b' -C3Repository: espressif/idf-eclipse-plugin
Length of output: 11266
🌐 Web query:
SWTBot setSelection exception WidgetNotFoundException IllegalArgumentException
💡 Result:
The exceptions WidgetNotFoundException and IllegalArgumentException when using SWTBot's setSelection (likely on SWTBotTree or SWTBotTable) typically occur due to timing, focus, or incorrect widget targeting issues. WidgetNotFoundException causes: - Widget not yet loaded or disposed: Common in trees when trying to access child nodes without expanding parent first (e.g., bot.tree.getTreeItem("Parent").getNode("Child").select fails; expand first: item.expand). - Wrong widget index: Multiple trees/tables present; use bot.tree(1) or bot.table(0) instead of bot.tree. - Timing/race conditions: Widget appears/disappears intermittently. Increase timeouts with -Dorg.eclipse.swtbot.playback.poll.delay=500 -Dorg.eclipse.swtbot.playback.delay=400, or use bot.sleep / waitUntil conditions. - Focus loss: Especially in debugger or headless (Xvfb); force shell activation: bot.activeShell.setFocus or UIThreadRunnable.syncExec to forceActive. - Linux-specific focus issues: Use Xephyr for tests. IllegalArgumentException causes: - Invalid index or argument to setSelection (e.g., out-of-bounds row/column index). - Underlying SWT widget state invalid (e.g., disposed, wrong thread access). - For trees: Path not fully expanded before selection. - Launch config issues: Missing SWTBot plugins or classloader problems in Eclipse tests. Solutions: 1. Ensure proper expansion: SWTBotTreeItem item = bot.tree.expandNode("Parent", "Child"); item.select; 2. Wait for widgets: bot.waitUntil(Conditions.shellIsActive("Title")); bot.waitUntil(new DefaultCondition { test { return bot.tree.getItems.length > 0; } }); 3. Use correct indices: Inspect with Eclipse Spy or print bot.widgets(WidgetOfType.widgetOfType(Tree.class)). 4. Activate contexts: bot.tabItem("Tab").activate; bot.tree(1).select(0); 5. JVM args for timing: As above. 6. For tables: Ensure column count matches; bot.table.select(rowIndex); Official docs confirm setSelection throws WidgetNotFoundException if widget null/disposed, and operations fail if not enabled/found.
Citations:
- 1: https://stackoverflow.com/questions/21463868/eclipse-swtbot-randomly-throws-widget-not-found-exception
- 2: https://wiki.eclipse.org/SWTBot/Troubleshooting
- 3: https://community.bonitasoft.com/t/little-trick-with-swtbottree-manipulation-if-it-returns-an-illegalargumentexception/11938
- 4: https://download.eclipse.org/technology/swtbot/helios/dev-build/apidocs/org/eclipse/swtbot/swt/finder/widgets/SWTBotTreeItem.html
- 5: https://stackoverflow.com/questions/30917195/swtbot-could-not-find-node-with-text-dynamic-web-project
- 6: https://stackoverflow.com/questions/19513994/trying-a-get-a-tree-item-in-tabitem-in-swtbot-but-it-is-not-working
- 7: https://www.eclipse.org/forums/index.php?t=msg&th=159683&goto=504781
🌐 Web query:
SWTBotCombo setSelection API documentation Eclipse SWTBot
💡 Result:
The SWTBotCombo class in Eclipse SWTBot provides the setSelection(String text) method to set the combo box selection to the specified text. public void setSelection(String text) Set the selection to the specified text. Parameters: text - the text to set into the combo. This is documented in the official SWTBot API documentation. Usage example from tests and tutorials: SWTBotCombo combo = bot.comboBox; // or bot.comboBoxWithLabel("Label"); combo.setSelection("Option Text"); There is also typically a setSelection(int index) method inherited or available for index-based selection, and setText(String text) for editable combos, as seen in recorder tests: bot.comboBox.setText("value"); The API is stable across versions, with the primary method being setSelection(String text) for text matching.
Citations:
- 1: https://download.eclipse.org/technology/swtbot/helios/dev-build/apidocs/org/eclipse/swtbot/swt/finder/widgets/SWTBotCombo.html
- 2: https://download.eclipse.org/technology/swtbot/galileo/dev-build/apidocs/org/eclipse/swtbot/swt/finder/SWTBot.html
- 3: https://git.eclipse.org/c/swtbot/org.eclipse.swtbot.git/tree/org.eclipse.swtbot.generator.test/src/org/eclipse/swtbot/generator/test/RecordComboTest.java?id=4946c6c9f846cd4c5a81aa397f41ca43259cf8c7
- 4: https://context7.com/eclipse-swtbot/org.eclipse.swtbot/llms.txt
🌐 Web query:
site:git.eclipse.org SWTBotCombo setSelection source code
💡 Result:
The source code for SWTBotCombo.setSelection in Eclipse SWTBot is located in the official GitHub repository at https://github.com/eclipse-swtbot/org.eclipse.swtbot. SWTBotCombo is a widget class in org.eclipse.swtbot.swt.finder.widgets package that wraps SWT Combo or CCombo controls. The setSelection method typically delegates to the underlying SWT widget's setSelection by index or text, using SWTBot's standard pattern of UIThreadRunnable.syncExec for thread-safe execution. SWTBot repositories have moved from git.eclipse.org cgit to GitHub (as noted in multiple sources). To view the exact implementation: 1. Navigate to https://github.com/eclipse-swtbot/org.eclipse.swtbot 2. Search for SWTBotCombo.java in the swt.finder module (likely examples/swtbot-eclipse/ or plugins/org.eclipse.swtbot.swt.finder/src/org/eclipse/swtbot/swt/finder/widgets/) 3. Look for public void setSelection(int index) or setSelection(String text) methods. Related evidence from Eclipse git mirrors: - Tests use bot.comboBox.setText("kikoo") for Combo interaction (source 1 from first search). - SWTBot generator rules include ComboSelectionRule and CComboSelectionRule, indicating SWTBotCombo likely handles both Combo and CCombo selection (source 9 from second search). - SWT CCombo has setSelection(Point selection) on its text field (source 2). No exact code snippet found in search highlights, but the GitHub repo is the authoritative up-to-date source per Eclipse migration notices (sources 6,7 across searches).
Citations:
- 1: https://git.eclipse.org/c/swtbot/org.eclipse.swtbot.git/tree/org.eclipse.swtbot.generator.test/src/org/eclipse/swtbot/generator/test/RecordComboTest.java?id=4946c6c9f846cd4c5a81aa397f41ca43259cf8c7
- 2: https://git.eclipse.org/c/platform/eclipse.platform.swt.git/blame/bundles/org.eclipse.swt/Eclipse%20SWT%20Custom%20Widgets/common/org/eclipse/swt/custom/CCombo.java
- 3: https://git.eclipse.org/c/swtbot/org.eclipse.swtbot.git/tree/
- 4: https://git.eclipse.org/c/swtbot/org.eclipse.swtbot.git/tree/org.eclipse.swtbot.generator
- 5: https://git.eclipse.org/c/swtbot/org.eclipse.swtbot.git/tree/org.eclipse.swtbot.generator/src/org/eclipse/swtbot/generator/framework/rules/SWTBotGeneratorRules.java
Narrow the caught exception type to avoid masking real widget failures.
The broad catch (Exception ignored) can hide unrelated SWTBot issues (e.g., widget not found, dialog didn't appear as expected, combo label changed) that occur in setSelection(portPrefixOrExact). When such failures occur, the exception is swallowed and the fallback prefix-scan runs instead, risking selection of the wrong port because startsWith may match a different board's port. The original failure never gets reported.
Consider either:
- Doing the prefix lookup first (deterministic and cheaper) and only falling back to exact
setSelectionif no prefix matches, or - Narrowing the caught type to exceptions actually thrown by SWTBot when an item is absent (e.g.,
IllegalArgumentExceptionor similar), and at minimum logging the swallowed exception so CI output captures what triggered the fallback.
Description
Since the number of ESP targets is constantly growing, I’d like to suggest a way to save time on release testing.
We already have an SWT test that builds and flashes the ESP32 target. We should expand this test to cover additional targets (esp32s2 / esp32s3 / esp32p4 / esp32c5 / esp32c6 /...) . I have all the necessary boards at my desk, so I could connect them to our Linux runner to test this scenario.
Fixes # (IEP-XXX)
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:
Dependent components impacted by this PR:
Checklist
Summary by CodeRabbit
Tests
Chores