WIP: SWTBot: add additional target support for the Build & Flash test case.#1401
Open
AndriiFilippov wants to merge 1 commit intomasterfrom
Open
WIP: SWTBot: add additional target support for the Build & Flash test case.#1401AndriiFilippov wants to merge 1 commit intomasterfrom
AndriiFilippov wants to merge 1 commit intomasterfrom
Conversation
📝 WalkthroughWalkthroughThis PR refactors two test utilities to support multi-target sequential flash testing. NewEspressifIDFProjectFlashProcessTest introduces a TARGETS collection with iterative flash flows and new helper methods for target switching and port selection. LaunchBarTargetSelector is enhanced with scrolling and visibility handling logic while maintaining public API signatures. Changes
Sequence DiagramsequenceDiagram
participant Test as Test Runner
participant Build as Build System
participant Port as Serial Port Selector
participant Flash as Flash Operation
participant Verify as Verification
loop For Each Target in TARGETS
Test->>Test: whenChangeLaunchTarget (optional)
activate Test
Test->>Port: Update target selection
deactivate Test
Test->>Build: whenBuildAndFlash
activate Build
Build->>Build: Build project
deactivate Build
Test->>Port: whenSelectLaunchTargetSerialPort
activate Port
Port->>Port: Try direct selection
alt Selection Failed
Port->>Port: Find matching port by prefix
end
Port-->>Test: Port selected
deactivate Port
Test->>Flash: Initiate flash
activate Flash
Flash->>Flash: Flash firmware
deactivate Flash
Test->>Verify: Verify flash result
activate Verify
Verify->>Verify: Check operation completion
deactivate Verify
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)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. 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: 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.
Comment on lines
+137
to
171
| 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.
Comment on lines
+160
to
+163
| } 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).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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
Release Notes