IEP-1649 Add SWT Tests to Verify ESP-IDF Terminal in the IDE #1337
Conversation
WalkthroughAdds a new SWTBot-based JUnit UI test class that opens the Eclipse Terminal view, runs Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Test as TerminalViewTest
participant SWTBot as SWTBot Harness
participant Terminal as Terminal View (Canvas)
participant Temp as Temp File
Note over Test,SWTBot: Setup
Test->>SWTBot: initialize SWTBot environment (`@BeforeClass`)
SWTBot-->>Test: ready
Note over Test,Terminal: Open terminal & prepare
Test->>Terminal: open Terminal view, select ESP-IDF Terminal
Terminal-->>Test: view opened / canvas available
Note over Test,Temp: Create temp file
Test->>Temp: create temporary file
Temp-->>Test: path
Note over Test,Terminal: Execute command
Test->>Terminal: send key events: type "idf.py --version > <temp>" + Enter
Terminal->>Temp: write command output (async)
Note over Test,Temp: Verify output
Test->>Temp: poll/read contents
Temp-->>Test: contents
Test->>Test: assert contains "IDF v"
Test->>Temp: delete temp file
Note over Test,SWTBot: Cleanup
Test->>SWTBot: close Terminal view if open (`@AfterClass`)
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Potential focus areas:
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/TerminalViewTest.java (3)
22-24: Consider adding class-level documentation.Add a javadoc comment explaining the purpose of this test class, what it verifies, and any prerequisites or assumptions (e.g., ESP-IDF must be configured, terminal implementation must support shell commands).
40-42: Use try-finally for reliable temp file cleanup.While
deleteOnExit()provides cleanup on JVM exit, using a try-finally block ensures the temp file is deleted even if assertions fail. This prevents accumulation of temp files during test development and debugging.Apply this diff:
- // Create a temporary file for capturing terminal output - Path tempFile = Files.createTempFile("idf_version_output", ".txt"); - File outputFile = tempFile.toFile(); - outputFile.deleteOnExit(); // ensure cleanup after JVM exits - - bot.toolbarButtonWithTooltip("Open a Terminal").click(); + // Create a temporary file for capturing terminal output + Path tempFile = Files.createTempFile("idf_version_output", ".txt"); + File outputFile = tempFile.toFile(); + + try { + bot.toolbarButtonWithTooltip("Open a Terminal").click();Then wrap the rest of the test method content and add finally at the end before line 95:
assertTrue("Output should contain 'IDF v'", output.contains("IDF v")); - // Optional: delete the temp file explicitly after test - Files.deleteIfExists(tempFile); + } finally { + // Delete the temp file explicitly after test + Files.deleteIfExists(tempFile); + } }
60-84: Consider extracting command typing logic to a helper method.The canvas key event simulation is verbose and would likely be reused in other terminal tests. Extracting it improves maintainability and testability.
Example helper method to add to the class:
/** * Types a command into the terminal canvas and presses Enter. * @param canvas The terminal canvas widget * @param command The command string to type */ private void typeCommandInTerminal(SWTBotCanvas canvas, String command) { canvas.display.syncExec(() -> { for (char c : command.toCharArray()) { Event e = new Event(); e.type = SWT.KeyDown; e.character = c; canvas.widget.notifyListeners(SWT.KeyDown, e); Event e2 = new Event(); e2.type = SWT.KeyUp; e2.character = c; canvas.widget.notifyListeners(SWT.KeyUp, e2); } // Press Enter Event enterDown = new Event(); enterDown.type = SWT.KeyDown; enterDown.character = '\r'; canvas.widget.notifyListeners(SWT.KeyDown, enterDown); Event enterUp = new Event(); enterUp.type = SWT.KeyUp; enterUp.character = '\r'; canvas.widget.notifyListeners(SWT.KeyUp, enterUp); }); }Then simplify the test to:
typeCommandInTerminal(canvas, cmd);
📜 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/TerminalViewTest.java(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/TerminalViewTest.java (1)
tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/operations/EnvSetupOperations.java (1)
EnvSetupOperations(15-101)
⏰ 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
🔇 Additional comments (2)
tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/TerminalViewTest.java (2)
28-34: LGTM!The setup method correctly initializes the SWTBot and configures the ESP-IDF environment.
59-59: ****The terminal uses Eclipse TM's
ProcessConnectorwhich spawns OS native shells (bash/sh on Unix, cmd.exe on Windows) that fully support I/O redirection. The concern about shell redirection failing is unfounded.However, the test does have other legitimate issues worth addressing:
- Fixed 5-second sleep may be insufficient or wasteful
- No verification that output file was actually created or written before asserting
- Race condition risk if command execution takes longer than 5 seconds
Likely an incorrect or invalid review comment.
tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/TerminalViewTest.java
Outdated
Show resolved
Hide resolved
tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/TerminalViewTest.java
Show resolved
Hide resolved
| // Read the output from the temporary file | ||
| String output = new String(Files.readAllBytes(tempFile), StandardCharsets.UTF_8); | ||
|
|
||
| assertTrue("Output should contain 'IDF v'", output.contains("IDF v")); |
There was a problem hiding this comment.
Add verification that the file was written before asserting content.
The test reads the file and asserts it contains "IDF v" without checking if:
- The file is non-empty
- The command actually executed
- The command succeeded vs. failed
If the file is empty or the command failed, the assertion message "Output should contain 'IDF v'" doesn't help diagnose the root cause.
Apply this diff to add pre-assertion checks:
// Read the output from the temporary file
String output = new String(Files.readAllBytes(tempFile), StandardCharsets.UTF_8);
+ assertTrue("Output file should not be empty - command may have failed",
+ !output.trim().isEmpty());
assertTrue("Output should contain 'IDF v'", output.contains("IDF v"));Alternatively, combine into a more informative message:
- assertTrue("Output should contain 'IDF v'", output.contains("IDF v"));
+ assertTrue(
+ String.format("Expected output to contain 'IDF v' but got: '%s'",
+ output.trim().isEmpty() ? "<empty>" : output.substring(0, Math.min(100, output.length()))),
+ output.contains("IDF v"));📝 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.
| // Read the output from the temporary file | |
| String output = new String(Files.readAllBytes(tempFile), StandardCharsets.UTF_8); | |
| assertTrue("Output should contain 'IDF v'", output.contains("IDF v")); | |
| // Read the output from the temporary file | |
| String output = new String(Files.readAllBytes(tempFile), StandardCharsets.UTF_8); | |
| assertTrue("Output file should not be empty - command may have failed", | |
| !output.trim().isEmpty()); | |
| assertTrue("Output should contain 'IDF v'", output.contains("IDF v")); |
🤖 Prompt for AI Agents
In
tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/TerminalViewTest.java
around lines 89-92, add pre-assertion checks so the temporary file was actually
written and the command likely succeeded before asserting content: verify the
file is non-empty (e.g. Files.size(tempFile) > 0 or output.length() > 0) and
include the actual file contents in the assertion message for easier diagnosis;
if you have access to the process/command result also assert the process exit
code is zero or that stderr is empty, then replace the existing assertTrue with
a more informative assertion that includes the output (e.g. "Expected output to
contain 'IDF v', actual output: " + output).
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/TerminalViewTest.java (3)
76-77: Replace fixed sleep with proper wait condition.The 3-second fixed sleep is brittle and unreliable across different system speeds. Use
TestWidgetWaitUtility.waitForOperationsInProgressToFinishSync(bot)aftersetFocus()for deterministic waiting.This concern was previously raised in past reviews. Based on learnings from
EnvSetupOperations.java, the pattern is:- // Wait for terminal to initialize - bot.sleep(3000); + // Wait for terminal to initialize + TestWidgetWaitUtility.waitForOperationsInProgressToFinishSync(bot);
111-112: Replace fixed sleep with polling for command completion.The 5-second fixed sleep creates a race condition and wastes time. Poll for the file content with a timeout instead of a fixed delay. This allows the test to proceed as soon as the command completes, while still having a safety timeout for failures.
This concern was previously raised. Consider this polling approach:
- // Wait for the command to execute - bot.sleep(5000); + // Wait for the command to write output (poll with timeout) + int maxWaitMs = 10000; + int pollIntervalMs = 500; + int elapsed = 0; + + while (elapsed < maxWaitMs) { + if (outputFile.exists() && outputFile.length() > 0) { + bot.sleep(200); // brief delay to ensure write completes + break; + } + bot.sleep(pollIntervalMs); + elapsed += pollIntervalMs; + }
114-117: Verify file was written before asserting content.The test reads the file without checking if it exists or is non-empty. If the command fails or the file is empty, the assertion message "Output should contain 'IDF v'" won't help diagnose the root cause.
This concern was previously raised. Add pre-assertion verification:
// Read the output from the temporary file String output = new String(Files.readAllBytes(tempFile), StandardCharsets.UTF_8); + assertTrue("Output file should not be empty - command may have failed", + !output.trim().isEmpty()); assertTrue("Output should contain 'IDF v'", output.contains("IDF v"));Or use a more informative single assertion:
- assertTrue("Output should contain 'IDF v'", output.contains("IDF v")); + assertTrue( + String.format("Expected output to contain 'IDF v' but got: '%s'", + output.trim().isEmpty() ? "<empty>" : output.substring(0, Math.min(200, output.length()))), + output.contains("IDF v"));
📜 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/TerminalViewTest.java(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/TerminalViewTest.java (1)
tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/operations/EnvSetupOperations.java (1)
EnvSetupOperations(15-101)
⏰ 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 (6)
tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/TerminalViewTest.java (6)
1-22: LGTM!Package declaration and imports are appropriate for a SWTBot-based UI test.
24-26: LGTM!Class declaration and annotation are correct for SWTBot testing.
41-67: LGTM!Temp file creation and toolbar button finding logic are well-implemented:
- Proper use of temp file APIs with cleanup hooks
- Defensive iteration through tooltip variations handles platform differences elegantly
- Clear error message if button not found
69-74: LGTM!Terminal view opening sequence follows standard SWTBot patterns correctly.
79-109: Verify terminal input simulation works across all target platforms.The low-level approach of manually creating
SWT.KeyDown/KeyUpevents and notifying canvas listeners directly bypasses normal terminal input handling. While this may be necessary given SWTBot's limited terminal support, it's inherently fragile and platform-dependent. Shell redirection (>) might not work in all environments, and there's no verification that the terminal actually processed the input.Per the PR description checklist, verify this test passes on Windows, Linux, and macOS. Consider adding a check that the command was actually received by the terminal (e.g., verify the temp file exists and has non-zero size before reading it).
118-119: LGTM!Explicit cleanup of the temporary file is good practice, especially combined with the
deleteOnExit()hook.
tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/TerminalViewTest.java
Outdated
Show resolved
Hide resolved
tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/TerminalViewTest.java
Show resolved
Hide resolved
sigmaaa
changed the title
|
@sigmaaa hi ! LGTM 👍 |
AndriiFilippov
force-pushed
the
IEP-1649
branch
from
2bf9d47 to
17da11c
Compare
AndriiFilippov
force-pushed
the
IEP-1649
branch
from
17da11c to
39b6878
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/TerminalViewTest.java (2)
91-93: Replace fixed 3s sleep with a deterministic wait for terminal readiness
bot.sleep(3000)makes the test timing-sensitive and potentially flaky on slower/faster environments. Prefer an explicit wait on a terminal-ready condition (for example, using existing wait utilities used in other UI tests or awaitUntilthat observes the ESP-IDF prompt/canvas state) instead of a hard-coded delay.
126-132: Avoid fixed 5s sleep; poll for output and strengthen assertionsThe 5-second sleep introduces a race (command may be slower or faster) and the single
"IDF v"assertion gives limited diagnostics if the command fails or the file is empty. Polling the output file with a timeout and asserting it’s non-empty before checking contents will make the test both more robust and easier to debug.- // Wait for the command to execute - bot.sleep(5000); - - // Read the output from the temporary file - String output = new String(Files.readAllBytes(tempFile), StandardCharsets.UTF_8); - - assertTrue("Output should contain 'IDF v'", output.contains("IDF v")); + // Wait for the command to write output (poll with timeout) + String output = ""; + int maxWaitMs = 10000; + int pollIntervalMs = 500; + int waited = 0; + + while (waited < maxWaitMs) + { + if (outputFile.exists() && outputFile.length() > 0) + { + output = new String(Files.readAllBytes(tempFile), StandardCharsets.UTF_8); + if (!output.trim().isEmpty()) + { + break; + } + } + bot.sleep(pollIntervalMs); + waited += pollIntervalMs; + } + + assertTrue("Output file should not be empty - command may have failed", !output.trim().isEmpty()); + assertTrue("Output should contain 'IDF v'", output.contains("IDF v"));
📜 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/TerminalViewTest.java(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/TerminalViewTest.java (1)
tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/operations/EnvSetupOperations.java (1)
EnvSetupOperations(15-101)
🔇 Additional comments (3)
tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/TerminalViewTest.java (3)
29-37: Static SWTBot setup and shared environment look correctUsing a static
SWTWorkbenchBotinitialized in a static@BeforeClassand delegating heavy UI/environment setup toEnvSetupOperations.setupEspressifEnv(bot)is aligned with typical SWTBot patterns and avoids redundant setup per test.
39-51: Class-level cleanup of the Terminal view is appropriateThe
@AfterClass cleanup()method that attempts to close the "Terminal" view and safely ignoresWidgetNotFoundExceptionis a good addition for keeping subsequent UI tests isolated.
61-77: Resilient lookup of the Terminal toolbar buttonUsing an array of possible tooltips and iterating until one matches is a nice way to handle shortcut-label variations across platforms/versions while still failing fast with a clear message when none is found.
| // Create a temporary file for capturing terminal output | ||
| Path tempFile = Files.createTempFile("idf_version_output", ".txt"); | ||
| File outputFile = tempFile.toFile(); | ||
| outputFile.deleteOnExit(); // ensure cleanup after JVM exits | ||
|
|
There was a problem hiding this comment.
Quote the temp file path in the shell redirection command
outputFile.getAbsolutePath() can contain spaces (very common on Windows home directories), so idf.py --version > <path> will misparse and likely fail. Wrap the path in quotes when building cmd so redirection works on all platforms.
- // Command with temporary file
- String cmd = "idf.py --version > " + outputFile.getAbsolutePath();
+ // Command with temporary file (quote path to handle spaces)
+ String cmd = "idf.py --version > \"" + outputFile.getAbsolutePath() + "\"";Also applies to: 98-100
🤖 Prompt for AI Agents
In
tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/TerminalViewTest.java
around lines 56-60 (and also apply the same change at lines 98-100), the
temporary output file path is used raw in a shell redirection which can break if
the path contains spaces; update the code that builds the command so the
outputFile.getAbsolutePath() is wrapped in quotes (e.g., "..." ) when inserted
into the redirection portion of the cmd string, ensuring the path is properly
quoted on all platforms.
Description
Please include a summary of the change and which issue is fixed.
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
✏️ Tip: You can customize this high-level summary in your review settings.