feat: cherry pick optimization commit from the 4.0.0 #1367
Conversation
* feat: added a test to verify esp-idf terminal in the IDE * fix: on the remote IDE there is a different lable for terminal * fix: cover all possible tooltips * fix: remove static bot field * fix: return static field since it's needed for beforeClass * fix: closing terminal view after tests
WalkthroughSeven UI test classes were refactored to move per-test cleanup ( Changes
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 (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: 1
🧹 Nitpick comments (15)
tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/executable/cases/project/IDFProjectJTAGFlashTest.java (1)
55-73: Consider usingAssume.assumeTrue()instead ofassertTrue(true).The platform check pattern with
assertTrue(true)in the else branch is a no-op that doesn't clearly communicate test skipping. Using JUnit'sAssumeAPI would be more idiomatic and provide better test reporting.+import org.junit.Assume; + @Test public void givenNewProjectCreatedWhenSelectJTAGflashWhenBuiltThenCheckFlashedSuccessfully() throws Exception { - if (SystemUtils.IS_OS_LINUX) // temporary solution until new ESP boards arrive for Windows - { - Fixture.givenNewEspressifIDFProjectIsSelected("EspressIf", "Espressif IDF Project"); - ... - } - else - { - assertTrue(true); - } + Assume.assumeTrue("Skipping: JTAG flash test requires Linux until new ESP boards arrive for Windows", + SystemUtils.IS_OS_LINUX); + Fixture.givenNewEspressifIDFProjectIsSelected("EspressIf", "Espressif IDF Project"); + ... }tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/executable/cases/project/NewEspressifIDFProjectFlashProcessTest.java (2)
55-73: Same platform skip pattern - considerAssume.assumeTrue()for clarity.As noted in
IDFProjectJTAGFlashTest.java, usingAssume.assumeTrue()would provide clearer test skip semantics.
156-161: Visibility inconsistency:cleanupEnvironment()is package-private but called from outer class.This method is package-private (
static void) while similar methods in other Fixture classes areprivate static. The method works since it's accessed from the enclosing class, but consider making visibility consistent across all test fixtures.- static void cleanupEnvironment() + private static void cleanupEnvironment()tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/executable/cases/project/IDFProjectLaunchTargetEditorFunctionalityTest.java (1)
195-200: Visibility inconsistency withcleanupEnvironment().Same as noted in other files - this method is package-private while the pattern in other fixtures varies. Consider standardizing visibility.
tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/executable/cases/project/NewEspressifIDFProjectClangFilesTest.java (2)
88-89: Remove or explain the// !comment.The comment
// !on line 88 appears to be a leftover marker with no clear meaning. Either remove it or replace with a meaningful explanation.- @Test // ! + @Test public void shouldApplyClangFormatSettingsWhenAutoSaveIsEnabled() throws Exception
385-386: Remove leftover comment// Or "Apply and Close".This comment appears to be a development note that should be removed.
- preferencesShell.bot().button("Cancel").click(); // Or "Apply and Close" + preferencesShell.bot().button("Cancel").click();tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/executable/cases/project/NewEspressifIDFProjectSDKconfigTest.java (1)
80-85: Consider makingwhenSDKconfigFileOpenedViaContextMenuthenVerifiedthenClosed()private.This method is only called once internally at line 106 within
whenSDKconfigFileDeletedWhenBuildProjectThenSDKconfigFileGeneratedAndVerified()and has no external usages. Since it's not part of the public API, marking it as private would better reflect its internal-only usage pattern.tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/executable/cases/project/NewEspressifIDFProjectPartitionTableEditorTest.java (4)
37-52: Shared BUILT/CLEAN projects with class-level lifecycle look consistentUsing
BUILT_PROJECT/CLEAN_PROJECTplus@BeforeClassand@AfterClassto create and tear down projects once per class should significantly reduce runtime and is aligned with the newer Fixture pattern in other tests. Centralizing workspace cleanup inFixture.cleanupEnvironment()also helps avoid leftover projects between classes.One small maintainability note: the same
"Project1"/"Project2"names are now hard-coded in multiple test classes; if these ever need to change, consider extracting them to a shared test utility/constant to avoid divergence.
55-61: “Unbuilt project” scenario depends onfullCleansemantics
shouldShowInfoMessageWhenOpeningEditorOnUnbuiltProjectrelies onFixture.fullClean(CLEAN_PROJECT)putting the project into the same state as a never‑built project so that the Information dialog is shown. Earlier tests rebuildCLEAN_PROJECT, so this behavior depends on what “Project Full Clean” does (e.g., whether it removessdkconfig/ partition CSV).If “full clean” semantics vary across IDF versions or toolchains, this test could become fragile. It might be safer to either:
- Use a dedicated “never built” project for this test, or
- Explicitly delete/restore the specific artifacts that drive the info popup rather than relying solely on
fullClean.
64-88: Row add/delete tests shareBUILT_PROJECTstate
shouldAddRowToPartitionTableandshouldDeleteRowFromPartitionTableboth operate onBUILT_PROJECT, relying on the Fixture/helpers to make each test self-contained (open editor, mutate, assert, close).Given
MethodSorters.NAME_ASCENDING,shouldAddRow…runs beforeshouldDeleteRow…. IfProjectTestOperations.comparePartitionTableRowscompares against an internal baseline taken per editor instance, this is fine; but if it compares against some global or initial baseline, running these tests on the same project could couple their outcomes.If you see order-dependent behavior or future flakes, consider either:
- Resetting the partition table to a known template at the start of each test, or
- Using separate projects for add vs delete scenarios.
91-103: Persistence and CSV deletion flows may need explicit waitsIn
shouldPersistChangesAfterSaveAndReopen,Fixture.saveAndQuit()andFixture.deletePartitionCsv()perform UI actions that can trigger background workspace jobs:
saveAndQuit()clicks “Save and Quit” then “OK” but does not wait for any follow‑up jobs to complete before the test reopens the editor and asserts persistence.deletePartitionCsv()deletespartitions.csvvia the context menu and immediately continues without any wait for deletion to finish.To reduce UI test flakiness, consider:
- Adding
TestWidgetWaitUtility.waitForOperationsInProgressToFinishSync(bot)(or an equivalent wait) aftersaveAndQuit()and after the delete‑resources dialog is confirmed.- Optionally waiting for the relevant shell (editor / delete dialog) to close before continuing.
This keeps the tests deterministic even when workspace operations are slower (e.g., on CI).
Also applies to: 185-213
tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/executable/cases/project/NewEspressifIDFProjectSBOMTest.java (4)
38-54: Class-level SBOM fixtures and shared projects are coherentDefining
BUILT_PROJECT,CLEAN_PROJECT, andSBOM_OUTPUTplus using@BeforeClass/@AfterClassto create both projects once and clean everything up viaFixture.cleanupEnvironment()fits well with the new test pattern and should speed up the SBOM suite.As with the partition editor tests, the
"Project1"/"Project2"literals are now duplicated across classes; if these names ever need to change, consider centralizing them in a shared test utility to avoid subtle mismatches between suites.
69-86: Consider explicitly closing SBOM dialog after runs to avoid UI leakageIn
sbomShouldGenerateToConsoleWhenProjectIsBuiltandsbomShouldRedirectToFileAndConsoleShouldConfirm, the tests callFixture.openSbomTool()andFixture.runSbomTool(), then proceed to assertions and file checks, but never callFixture.closeSbomTool():
- If the SBOM dialog auto-closes when the job finishes, this is fine.
- If it stays open, the dialog could interfere with later tests (e.g., other dialogs, button lookups, or focus assumptions).
To make the tests more robust and self-contained, consider either:
- Having
runSbomTool()wait for the SBOM shell to close (and treating that as part of the helper’s contract), or- Explicitly invoking
Fixture.closeSbomTool()at the end of these tests once all assertions are done.Also applies to: 171-187
115-128: Prefer JUnit assumptions over “no-op pass” on Linux
spaceInOutputFilePathShouldDisableRunOnNonLinuxcurrently does:if (SystemUtils.IS_OS_LINUX) { assertTrue(true); return; }This means the test always passes on Linux rather than being reported as skipped/ignored, which can hide platform-specific coverage gaps.
Consider using a JUnit assumption instead, for example:
- if (SystemUtils.IS_OS_LINUX) - { - assertTrue(true); - return; - } + if (SystemUtils.IS_OS_LINUX) + { + org.junit.Assume.assumeFalse("Test only applicable on non-Linux platforms", true); + }(or the equivalent
assumeTrue(!SystemUtils.IS_OS_LINUX)before the rest of the test). This makes the intent explicit in test reports.
130-147: Add an explicit wait for the SBOM dialog to reduce flakiness
Fixture.openSbomTool()launches the “SBOM Tool” via the context menu but does not wait for the dialog shell to appear before subsequent operations start usingbot.shell(SBOM_DIALOG)andbot.button("OK").To align with the partition editor tests (which use
TestWidgetWaitUtility.waitForDialogToAppear), and to avoid flakiness on slower environments/CI, consider:static void openSbomTool(String projectName) throws IOException { - ProjectTestOperations.launchCommandUsingContextMenu(projectName, bot, "SBOM Tool"); + ProjectTestOperations.launchCommandUsingContextMenu(projectName, bot, "SBOM Tool"); + TestWidgetWaitUtility.waitForDialogToAppear(bot, SBOM_DIALOG, 10000); }This makes it much less likely that tests race the dialog creation.
Also applies to: 161-169
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/executable/cases/project/IDFProjectJTAGFlashTest.java(3 hunks)tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/executable/cases/project/IDFProjectLaunchTargetEditorFunctionalityTest.java(4 hunks)tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/executable/cases/project/NewEspressifIDFProjectClangFilesTest.java(7 hunks)tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/executable/cases/project/NewEspressifIDFProjectFlashProcessTest.java(5 hunks)tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/executable/cases/project/NewEspressifIDFProjectPartitionTableEditorTest.java(2 hunks)tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/executable/cases/project/NewEspressifIDFProjectSBOMTest.java(2 hunks)tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/executable/cases/project/NewEspressifIDFProjectSDKconfigTest.java(6 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-27T18:07:42.470Z
Learnt from: sigmaaa
Repo: espressif/idf-eclipse-plugin PR: 1339
File: bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/nvs/handlers/NvsEditorHandler.java:27-34
Timestamp: 2025-10-27T18:07:42.470Z
Learning: In the file bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/nvs/handlers/NvsEditorHandler.java, the NvsEditorHandler is tied to a project explorer right-click command, so EclipseUtil.getSelectedProjectInExplorer() will never return null due to the command's enablement conditions.
Applied to files:
tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/executable/cases/project/NewEspressifIDFProjectSDKconfigTest.java
🧬 Code graph analysis (4)
tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/executable/cases/project/NewEspressifIDFProjectFlashProcessTest.java (1)
tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/executable/cases/project/NewEspressifIDFProjectSBOMTest.java (1)
Fixture(130-218)
tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/executable/cases/project/IDFProjectJTAGFlashTest.java (2)
tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/executable/cases/project/NewEspressifIDFProjectFlashProcessTest.java (1)
Fixture(75-162)tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/executable/cases/project/NewEspressifIDFProjectSBOMTest.java (1)
Fixture(130-218)
tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/executable/cases/project/NewEspressifIDFProjectPartitionTableEditorTest.java (4)
tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/executable/cases/project/NewEspressifIDFProjectSBOMTest.java (1)
Fixture(130-218)tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/executable/cases/project/NewEspressifIDFProjectTest.java (1)
Fixture(193-444)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(15-101)
tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/executable/cases/project/NewEspressifIDFProjectSDKconfigTest.java (5)
tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/executable/cases/project/IDFProjectJTAGFlashTest.java (1)
Fixture(75-159)tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/executable/cases/project/NewEspressifIDFProjectFlashProcessTest.java (1)
Fixture(75-162)tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/executable/cases/project/NewEspressifIDFProjectPartitionTableEditorTest.java (1)
Fixture(105-220)tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/executable/cases/project/NewEspressifIDFProjectSBOMTest.java (1)
Fixture(130-218)tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/executable/cases/project/NewEspressifIDFProjectTest.java (1)
Fixture(193-444)
⏰ 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 (15)
tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/executable/cases/project/IDFProjectJTAGFlashTest.java (2)
49-53: Lifecycle change from per-test to class-level cleanup looks correct.The switch from
@Afterto@AfterClassis appropriate here since there's only one test method, and it aligns with the optimization goal of reducing build time by minimizing cleanup overhead.
153-158: Cleanup method implementation is consistent with other test classes.The
cleanupEnvironment()method follows the same pattern as other test fixtures in this PR (wait for operations, close projects, delete projects).tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/executable/cases/project/NewEspressifIDFProjectFlashProcessTest.java (1)
49-53: Class-level teardown appropriate for single-test class.The switch to
@AfterClassaligns with the optimization pattern used across this PR.tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/executable/cases/project/IDFProjectLaunchTargetEditorFunctionalityTest.java (3)
43-53: Project created once in @BeforeClass and shared across tests.The shared project pattern with
@FixMethodOrder(NAME_ASCENDING)creates an implicit dependency between tests. This is intentional for the optimization but worth noting that test failures may cascade.
55-72: Test ordering relies on alphabetical method names.The tests execute in order:
shouldBuildProject...beforeshouldDeleteBuildFolder.... The first test builds the project and verifies behavior on target change, while the second creates a new target and deletes it. This ordering dependency is maintained by@FixMethodOrder(NAME_ASCENDING).
182-193: UI-based sdkconfig deletion added towhenProjectFullCleaned().This method now explicitly deletes the sdkconfig file via context menu and waits for the dialog to close. This ensures a clean state for subsequent operations.
tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/executable/cases/project/NewEspressifIDFProjectSDKconfigTest.java (3)
43-47: Class-level teardown for single-test class is appropriate.With only one comprehensive test method, switching to
@AfterClassreduces cleanup overhead.
49-64: Consolidated test flow covers full SDKconfig lifecycle.The test now includes additional steps to close the shell and verify sdkconfig regeneration after deletion, providing more comprehensive coverage in a single test method.
214-219: Cleanup implementation consistent with other test fixtures.The
cleanupEnvironment()pattern matches the approach used across other test classes in this PR.tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/executable/cases/project/NewEspressifIDFProjectClangFilesTest.java (3)
46-55: Two shared projects created in @BeforeClass for multi-test class.Creating both projects upfront and sharing across tests is the correct optimization pattern. The tests use
CLEAN_PROJECT1andCLEAN_PROJECT2to distinguish operations.
102-110: Test order dependency:shouldMatchExpectedClangdArgumentsAfterBuildingProjectsbuilds both projects.Due to
@FixMethodOrder(NAME_ASCENDING), this test runs aftershouldApplyClangFormatSettingsWhenAutoSaveIsEnabledandshouldHaveClangFilesPresentAndContentCorrectForNewProject. It builds both projects, which is required for subsequent checks. Verify this ordering aligns with test intent.
389-394: Cleanup implementation consistent with pattern across PR.The
cleanupEnvironment()method follows the established pattern.tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/executable/cases/project/NewEspressifIDFProjectPartitionTableEditorTest.java (1)
110-140: Fixture env setup/cleanup is aligned with shared SWTBot pattern
Fixture.loadEnv()andFixture.cleanupEnvironment()follow the same pattern as other test fixtures (singleSWTWorkbenchBotviaWorkBenchSWTBot.getBot(), one‑timeEnvSetupOperations.setupEspressifEnv, and class-level project cleanup viaProjectTestOperations.closeAllProjects/deleteAllProjects). That should keep the workspace in a known state across this suite and avoid repeated environment setup cost.No issues from my side here; this is a good fit for the cherry‑picked optimization.
Also applies to: 214-219
tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/executable/cases/project/NewEspressifIDFProjectSBOMTest.java (2)
56-66: Non-built project disablement test reads clean and robust
sbomToolShouldBeDisabledWhenProjectIsNotBuiltexercises the SBOM dialog both with and without “Redirect output to the file” checked and consistently asserts the OK button is disabled viaFixture.shouldDisableRunButton().The helper’s use of
SWTBotButton ok = bot.button("OK");and checking!ok.isEnabled()keeps the assertion focused and reusable across tests. This is a nice consolidation of what used to be more verbose inline SWTBot code.Also applies to: 198-202
88-113: Validation tests and SBOM file verification helpers look goodThe tests
emptyDescriptionPathShouldDisableRun,spaceInDescriptionPathShouldDisableRun, andbackslashInDescriptionPathShouldDisableRunclearly express the validation behavior they exercise, and the corresponding Fixture helpers (setProjectDescriptionPath,shouldDisableRunButton,closeSbomTool) keep the interaction terse.Similarly,
openSbomFileandshouldSeeSbomFileContainsdelegate the editor open and content check to existing operations utilities, which should make future changes to the editor implementation easier to manage in one place.No functional concerns here.
Also applies to: 188-196, 209-217
| private static void thenCompareActualClangdDriversWithExpected() throws IOException | ||
| { | ||
| SWTBotShell prefrencesShell = bot.shell("Preferences"); | ||
| String actualClangdPath = prefrencesShell.bot().textWithLabel("Path").getText(); | ||
| String expectedClangdPath = "bin"; | ||
| assertTrue(actualClangdPath.contains(expectedClangdPath)); | ||
| String actualDriversPath = prefrencesShell.bot().textWithLabel("Drivers").getText(); | ||
| String expectedDriversPath = "**"; | ||
| assertEquals(expectedDriversPath, actualDriversPath); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Eclipse CDT LSP clangd drivers default configuration value
💡 Result:
The "Drivers" setting defaults to empty (no globs specified) — i.e., no drivers configured by CDT LSP (clangd will fall back to its usual behavior such as searching PATH for a gcc to infer include paths). [1][2]
Sources:
- Eclipse CDT LSP clangd preference description (Drivers = "A comma-separated list of globs..."). [1]
- clangd config docs (behavior when no query-driver / driver not supplied). [2]
🏁 Script executed:
# Examine the test file context around the flagged lines
find . -name "NewEspressifIDFProjectClangFilesTest.java" -type fRepository: espressif/idf-eclipse-plugin
Length of output: 202
🏁 Script executed:
# Read the test file to understand context around lines 356-362
wc -l ./tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/executable/cases/project/NewEspressifIDFProjectClangFilesTest.javaRepository: espressif/idf-eclipse-plugin
Length of output: 206
🏁 Script executed:
# Read the relevant section and surrounding context
sed -n '340,380p' ./tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/executable/cases/project/NewEspressifIDFProjectClangFilesTest.javaRepository: espressif/idf-eclipse-plugin
Length of output: 1949
🏁 Script executed:
# Search for where drivers path might be set or initialized in this test
rg -n "Drivers\|drivers\|\*\*" ./tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/executable/cases/project/NewEspressifIDFProjectClangFilesTest.java -A 2 -B 2Repository: espressif/idf-eclipse-plugin
Length of output: 54
🏁 Script executed:
# Search for setup methods and initialization in the test file
sed -n '1,100p' ./tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/executable/cases/project/NewEspressifIDFProjectClangFilesTest.javaRepository: espressif/idf-eclipse-plugin
Length of output: 3675
🏁 Script executed:
# Search for where the drivers preference might be set (grep around the whole test file)
cat -n ./tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/executable/cases/project/NewEspressifIDFProjectClangFilesTest.java | head -200Repository: espressif/idf-eclipse-plugin
Length of output: 8775
🏁 Script executed:
# Search broadly in the codebase for drivers configuration and "**" pattern
rg -n "\"\\*\\*\"" ./tests --type java -B 3 -A 3Repository: espressif/idf-eclipse-plugin
Length of output: 1267
🏁 Script executed:
# Search for clangd drivers preference initialization or configuration
rg -n "Drivers|drivers" ./src --type java -i | grep -i "pref\|config\|default" | head -20Repository: espressif/idf-eclipse-plugin
Length of output: 117
🏁 Script executed:
# Look for preference-related code in the main plugin
fd -name "*.java" -path "*/src/*" | xargs rg -l "Drivers" | head -10Repository: espressif/idf-eclipse-plugin
Length of output: 557
🏁 Script executed:
# Read the clangd support documentation
cat ./docs_readme/clangd_cdt_support.mdRepository: espressif/idf-eclipse-plugin
Length of output: 3493
🏁 Script executed:
# Also check the other documentation file
cat ./docs/en/additionalfeatures/clangd_cdt_support.rstRepository: espressif/idf-eclipse-plugin
Length of output: 4770
Extract the hardcoded drivers path "**" as a named constant with documentation.
The expected drivers path is hardcoded as "**" without explanation. Since Espressif plugin documentation states this value is "configured based on the selected target and project," extract it as a private static final String constant with a comment clarifying why this glob pattern is the expected value for Espressif projects.
🤖 Prompt for AI Agents
In
tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/executable/cases/project/NewEspressifIDFProjectClangFilesTest.java
around lines 356 to 362, the expected drivers path is hardcoded as the string
"**"; extract this literal into a private static final String constant (e.g.
EXPECTED_DRIVERS_GLOB) placed near the top of the class, add a brief JavaDoc
comment explaining that the "**" glob is used because the Espressif plugin
configures drivers based on selected target/project and therefore a recursive
glob is expected, and replace the hardcoded literal in
thenCompareActualClangdDriversWithExpected() with the new constant.
|
Hi @sigmaaa please verify the failing checks |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/executable/cases/project/NewEspressifIDFProjectClangFilesTest.java (1)
357-363: Extract the hardcoded drivers path"**"as a named constant.As noted in the previous review, the expected drivers value
"**"should be extracted to a constant with documentation explaining its purpose. This aligns with the Espressif plugin's configuration behavior.private static class Fixture { private static SWTWorkbenchBot bot; + /** + * Expected clangd drivers glob pattern. The Espressif plugin configures this + * based on the selected target and project to enable recursive driver discovery. + */ + private static final String EXPECTED_DRIVERS_GLOB = "**"; // ... later in thenCompareActualClangdDriversWithExpected(): - String expectedDriversPath = "**"; + String expectedDriversPath = EXPECTED_DRIVERS_GLOB;
🧹 Nitpick comments (1)
tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/executable/cases/project/NewEspressifIDFProjectClangFilesTest.java (1)
172-175: PreferassertFalseoverassertTrue(!...).Using
assertFalseis more readable and provides clearer assertion failure messages.private static void thenClangdFileIsAbsent(String projectName) throws IOException { - assertTrue(!bot.tree().getTreeItem(projectName).getNodes().contains(".clangd")); + assertFalse("Expected .clangd to be absent in " + projectName, + bot.tree().getTreeItem(projectName).getNodes().contains(".clangd")); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/executable/cases/project/NewEspressifIDFProjectClangFilesTest.java(11 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/executable/cases/project/NewEspressifIDFProjectClangFilesTest.java (4)
tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/executable/cases/project/NewEspressifIDFProjectSBOMTest.java (1)
Fixture(130-218)tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/executable/cases/project/NewEspressifIDFProjectTest.java (1)
Fixture(193-444)tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/operations/EnvSetupOperations.java (1)
EnvSetupOperations(15-101)tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/operations/ProjectTestOperations.java (1)
ProjectTestOperations(51-789)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build_windows
- GitHub Check: build_macos
🔇 Additional comments (5)
tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/executable/cases/project/NewEspressifIDFProjectClangFilesTest.java (5)
47-48: LGTM! Good use of constants for project names.Extracting project names as constants improves maintainability and makes tests easier to understand.
50-62: Verify test isolation with shared project state.Moving from per-test cleanup (
@After) to class-level cleanup (@AfterClass) improves performance but introduces shared state between tests. Since tests are ordered alphabetically via@FixMethodOrder(MethodSorters.NAME_ASCENDING), ensure:
- Test
shouldApplyClangFormatSettingsWhenAutoSaveIsEnabled(runs 2nd) modifiesCLEAN_PROJECT2, which is then built inshouldMatchExpectedClangdArgumentsAfterBuildingProjects(runs 3rd).- Test
shouldHaveClangFilesPresentAndContentCorrectForNewProject(runs 1st) andshouldRecreateClangdFileAfterDeletionAndVerifyContent(runs 2nd) both operate onCLEAN_PROJECT1.The test naming and project assignments appear to account for this ordering, but if test names change, the execution order will shift.
117-128: LGTM! Setup follows established patterns.The
loadEnv()andcreateProject()methods align with similar implementations in other test classes likeNewEspressifIDFProjectSBOMTest.java.
349-355: LGTM! Good use ofassertEqualsfor path comparison.The switch from
assertTruewith string comparison toassertEqualsimproves assertion failure messages by showing both expected and actual values.
390-395: LGTM! Cleanup follows established patterns.The
cleanupEnvironment()method correctly waits for pending operations and cleans up projects, matching the pattern used in other test classes likeNewEspressifIDFProjectSBOMTest.java.
| @Test // ! | ||
| public void shouldApplyClangFormatSettingsWhenAutoSaveIsEnabled() throws Exception |
There was a problem hiding this comment.
Remove the unexplained // ! comment.
The // ! comment appears to be a debug marker or incomplete annotation. Either remove it or replace it with a meaningful comment explaining the test's purpose.
- @Test // !
+ @Test
public void shouldApplyClangFormatSettingsWhenAutoSaveIsEnabled() throws Exception📝 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.
| @Test // ! | |
| public void shouldApplyClangFormatSettingsWhenAutoSaveIsEnabled() throws Exception | |
| @Test | |
| public void shouldApplyClangFormatSettingsWhenAutoSaveIsEnabled() throws Exception |
🤖 Prompt for AI Agents
In
tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/executable/cases/project/NewEspressifIDFProjectClangFilesTest.java
around lines 89 to 90, remove the unexplained "// !" trailing comment after the
@Test annotation; either delete it entirely or replace it with a concise,
meaningful comment describing the test’s intent (e.g., "// verifies clang-format
settings applied on autosave") so the code is clear and no stray debug markers
remain.
|
we don't need this PR as 4.0.0 will be merged to the master |
Description
Cherry-picked an optimization commit from the 4.0.0 branch. This should also fix the failing tests on master and reduce the overall build time by more than half.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.