Cherry pick from v4.0.0 #1381
Cherry pick from v4.0.0 #1381
Conversation
* ci: optimize tests to reduce execution time
sigmaaa
requested review from
AndriiFilippov,
Shen7436,
alirana01 and
kolipakakondal
📝 WalkthroughWalkthroughAdds LspService calls to apply clangd preferences during IDE tool setup; updates releng target to a newer LSP4E installable unit; inlines and rewrites several English/Chinese docs; adds a new UI startup listener; and refactors many SWTBot UI tests to class-level teardowns with centralized Fixture cleanup. Changes
Sequence Diagram(s)(omitted) Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧹 Recent nitpick comments
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
🔇 Additional comments (9)
✏️ Tip: You can disable this entire section by setting Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In
@bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/SetupToolsInIde.java:
- Around line 154-158: Wrap the LSP configuration block that constructs
LspService and calls updateClangdPath() and updateQueryDriver() in its own
try-catch so runtime/unchecked exceptions do not abort setup; specifically
surround the code that creates new LspService() and calls
lspService.updateClangdPath() and lspService.updateQueryDriver() with a try {
... } catch (RuntimeException | Exception e) { log("Applying Clangd Preferences
failed: " + e.getMessage(), IStatus.WARNING); /* optionally log stack trace */ }
(preserve monitor.setTaskName and the original log call on success) so failures
are logged and setup continues.
In @docs/en/additionalfeatures/clangd_cdt_support.rst:
- Around line 27-50: Update the broken image directives in the .rst: replace the
.. image:: ../../../media/clangd/cdtlsp_updatesite.png reference with .. image::
media/clangd_updatesite.png (the actual file name), ensure the existing ..
image:: ../../../media/clangd/clangd_config.png is left as-is, and either
correct the final .. image:: ../../../media/clangd/cdt_indexer_disable.png to
point to the real image filename in the media directory or add the missing
cdt_indexer_disable.png into the media folder and reference it from the
document; make these changes on the corresponding image directive lines so the
.. image:: directives point to valid files.
In
@tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/executable/cases/project/NewEspressifIDFProjectClangFilesTest.java:
- Around line 101-108: Add the missing @Test annotation to the method
shouldMatchExpectedClangdArgumentsAfterBuildingProjects so the JUnit runner
executes it; place @Test immediately above the method declaration and ensure the
matching Test import (e.g., org.junit.Test or org.junit.jupiter.api.Test) is
present in the file, adjusting other tests in this package to match the
project's JUnit version if needed.
- Around line 151-156: Remove the unused duplicate method
thenClangdPathUpdateOnSelectedTarget(): locate its declaration and delete the
entire private static void thenClangdPathUpdateOnSelectedTarget() method body
since its behavior is duplicated by thenClangdDriversUpdateOnSelectedTarget,
which is the method actually used; ensure no other references to
thenClangdPathUpdateOnSelectedTarget remain (if any, replace them with
thenClangdDriversUpdateOnSelectedTarget).
🧹 Nitpick comments (9)
tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/executable/cases/project/NewEspressifIDFProjectFlashProcessTest.java (1)
69-72: Consider using JUnit'sAssumefor conditional test execution.Using
assertTrue(true)as a no-op when skipping on non-Linux platforms is unconventional and can be misleading in test reports (shows as passed rather than skipped). JUnit'sAssume.assumeTrue()would be more appropriate.♻️ Suggested refactor
+import static org.junit.Assume.assumeTrue; ... @Test public void givenNewProjectCreatedBuiltWhenSelectSerialPortWhenFlashThenCheckFlashedSuccessfully() throws Exception { - if (SystemUtils.IS_OS_LINUX) // temporary solution until new ESP boards arrive for Windows - { + assumeTrue("Skipping: requires Linux with ESP boards", SystemUtils.IS_OS_LINUX); Fixture.givenNewEspressifIDFProjectIsSelected("EspressIf", "Espressif IDF Project"); Fixture.givenProjectNameIs("NewProjectFlashTest"); ... - } - else - { - assertTrue(true); - } }tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/executable/cases/project/IDFProjectJTAGFlashTest.java (1)
69-72: Same pattern issue: preferAssume.assumeTrue()for conditional test skipping.As noted in the companion file,
assertTrue(true)masks the skip reason. UsingAssume.assumeTrue("Skipping: requires Linux", SystemUtils.IS_OS_LINUX)would report the test as "ignored" rather than "passed".tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/executable/cases/project/NewEspressifIDFProjectSDKconfigTest.java (1)
73-78: Missing initial project cleanup inloadEnv().Unlike other test fixtures in this PR (e.g.,
IDFProjectJTAGFlashTest,NewEspressifIDFProjectFlashProcessTest), thisloadEnv()does not callProjectTestOperations.deleteAllProjects(bot)after environment setup. This could cause test failures if leftover projects exist from prior test runs.♻️ Add deleteAllProjects for consistency
private static void loadEnv() throws Exception { bot = WorkBenchSWTBot.getBot(); EnvSetupOperations.setupEspressifEnv(bot); bot.sleep(1000); + ProjectTestOperations.deleteAllProjects(bot); }tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/executable/cases/project/NewEspressifIDFProjectPartitionTableEditorTest.java (1)
110-115: Missing initial project cleanup inloadEnv().For consistency with other fixtures in this PR and to ensure a clean starting state, consider adding
ProjectTestOperations.deleteAllProjects(bot)after environment setup.♻️ Add deleteAllProjects
static void loadEnv() throws Exception { bot = WorkBenchSWTBot.getBot(); EnvSetupOperations.setupEspressifEnv(bot); bot.sleep(1000); + ProjectTestOperations.deleteAllProjects(bot); }tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/executable/cases/project/NewEspressifIDFProjectSBOMTest.java (2)
115-128: Same pattern issue: preferAssume.assumeTrue()for conditional test skipping.The
assertTrue(true); return;pattern should be replaced withAssume.assumeTrue()for proper test reporting.♻️ Suggested refactor
+import static org.junit.Assume.assumeTrue; ... @Test public void spaceInOutputFilePathShouldDisableRunOnNonLinux() throws Exception { - if (SystemUtils.IS_OS_LINUX) - { - assertTrue(true); - return; - } + assumeTrue("This validation only applies on non-Linux", !SystemUtils.IS_OS_LINUX); Fixture.openSbomTool(BUILT_PROJECT); ... }
135-147: Missing initial project cleanup inloadEnv().For consistency with other test fixtures, consider adding
ProjectTestOperations.deleteAllProjects(bot)to ensure a clean workspace.♻️ Add deleteAllProjects
static void loadEnv() throws Exception { bot = WorkBenchSWTBot.getBot(); EnvSetupOperations.setupEspressifEnv(bot); bot.sleep(1000); + ProjectTestOperations.deleteAllProjects(bot); }tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/executable/cases/project/IDFProjectLaunchTargetEditorFunctionalityTest.java (2)
78-78: Consider potential project name conflicts.The hardcoded project name
"Project"could cause issues if:
- Multiple test classes run concurrently and use the same name
- A previous test run didn't clean up properly
Consider using a unique name (e.g., with timestamp or test class name prefix) to avoid conflicts.
182-193: Method name doesn't fully reflect its behavior.The method
whenProjectFullCleaned()performs a full clean operation but also explicitly deletes thesdkconfigfile (lines 189-192). Consider either:
- Renaming to
whenProjectFullCleanedAndSdkconfigDeleted()for clarity- Extracting the sdkconfig deletion into a separate method
This improves maintainability by making the method's complete behavior obvious from its name.
tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/executable/cases/project/NewEspressifIDFProjectClangFilesTest.java (1)
87-88: Remove leftover debug marker.The
// !comment appears to be a debug artifact that should be removed.🧹 Proposed fix
- @Test // ! + @Test public void shouldApplyClangFormatSettingsWhenAutoSaveIsEnabled() throws Exception
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/SetupToolsInIde.javadocs/en/additionalfeatures/clangd_cdt_support.rstdocs/en/additionalfeatures/dfu.rstdocs/en/additionalfeatures/wokwisimulator.rstdocs/zh_CN/additionalfeatures/clangd_cdt_support.rstdocs/zh_CN/additionalfeatures/dfu.rstdocs/zh_CN/additionalfeatures/wokwisimulator.rstreleng/com.espressif.idf.target/com.espressif.idf.target.targettests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/executable/cases/project/IDFProjectJTAGFlashTest.javatests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/executable/cases/project/IDFProjectLaunchTargetEditorFunctionalityTest.javatests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/executable/cases/project/NewEspressifIDFProjectClangFilesTest.javatests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/executable/cases/project/NewEspressifIDFProjectFlashProcessTest.javatests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/executable/cases/project/NewEspressifIDFProjectPartitionTableEditorTest.javatests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/executable/cases/project/NewEspressifIDFProjectSBOMTest.javatests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/executable/cases/project/NewEspressifIDFProjectSDKconfigTest.java
🧰 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 (3)
tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/executable/cases/project/NewEspressifIDFProjectClangFilesTest.java (3)
tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/common/WorkBenchSWTBot.java (1)
WorkBenchSWTBot(14-28)tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/operations/EnvSetupOperations.java (1)
EnvSetupOperations(16-89)tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/operations/ProjectTestOperations.java (1)
ProjectTestOperations(51-789)
tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/executable/cases/project/NewEspressifIDFProjectSDKconfigTest.java (3)
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/NewEspressifIDFProjectClangFilesTest.java (1)
Fixture(110-400)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/NewEspressifIDFProjectSBOMTest.java (4)
tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/executable/cases/project/NewEspressifIDFProjectTest.java (1)
Fixture(144-395)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(16-89)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). (3)
- GitHub Check: build_linux
- GitHub Check: build_macos
- GitHub Check: build_windows
🔇 Additional comments (46)
tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/executable/cases/project/NewEspressifIDFProjectFlashProcessTest.java (3)
7-9: LGTM!Static imports are correctly added for
widgetIsEnabled(used inwhenFlashProject) andassertTrue(used in the test method).
49-53: LGTM!The transition from instance-level
@Afterto class-level@AfterClasswith statictearDown()is appropriate here since there's only one test method, and the cleanup delegates toFixture.cleanupEnvironment()consistently with other tests in the PR.
156-161: LGTM!The
cleanupEnvironment()method follows the established pattern: wait for async operations, close all projects, then delete all projects. This ensures proper test isolation.tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/executable/cases/project/IDFProjectJTAGFlashTest.java (3)
8-10: LGTM!Static imports are appropriately added and used within the test class.
49-53: LGTM!Class-level teardown pattern is consistent with other tests in the PR.
153-158: LGTM!Cleanup logic is consistent with the established fixture pattern across the test suite.
tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/executable/cases/project/NewEspressifIDFProjectSDKconfigTest.java (3)
14-14: LGTM!The lifecycle change to
@AfterClasswith statictearDown()aligns with the broader test refactoring pattern in this PR.Also applies to: 43-47
49-64: LGTM!The consolidated test method provides good coverage of the SDKconfig workflow: create → build → verify presence → edit/save → rebuild → verify persistence → delete/regenerate.
214-219: LGTM!Cleanup method follows the established pattern.
tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/executable/cases/project/NewEspressifIDFProjectPartitionTableEditorTest.java (6)
13-14: LGTM!New imports and constants are well-structured. Using named constants for project names improves readability and maintainability.
Also applies to: 37-39
40-46: LGTM!Creating both built and clean projects in
@BeforeClassis efficient—it avoids redundant project creation across tests while providing the appropriate fixture state for each test case.
48-52: LGTM!Class-level teardown is appropriate and consistent with the PR pattern.
54-103: Test ordering dependency with@FixMethodOrder.These tests use
NAME_ASCENDINGordering, soshouldAddRowToPartitionTableruns beforeshouldDeleteRowFromPartitionTable. However, both tests operate onBUILT_PROJECTwithout resetting state between them. IfshouldAddRowToPartitionTableadds a row,shouldDeleteRowFromPartitionTablemay delete that added row rather than an original row, which could cause flaky assertions depending on expected row counts.Consider whether tests should reset the partition table state or use separate projects for isolation.
165-173: LGTM!The
addRow()andassertRowAdded()helpers provide clean abstractions for partition table manipulation.
206-212: LGTM!Good practice to clean up the generated
partitions.csvafter the persistence test to avoid affecting subsequent test runs.tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/executable/cases/project/NewEspressifIDFProjectSBOMTest.java (4)
15-15: LGTM!Good use of constants for project names and expected output. The
SBOM_OUTPUT = "SPDXVersion"constant clearly documents what the test verifies in the SBOM output.Also applies to: 38-41
42-54: LGTM!Lifecycle setup is consistent with the PR pattern. Creating both built and clean projects upfront is efficient for the test scenarios.
198-202: LGTM!Good assertion helper with a clear error message. The negation pattern
!ok.isEnabled()withassertTrueworks correctly.
166-186: LGTM!The SBOM tool interaction helpers (
openSbomTool,closeSbomTool,toggleRedirectOutput,runSbomTool) provide clean abstractions. UsingSBOM_DIALOGconstant forbot.shell()calls ensures consistency.tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/executable/cases/project/IDFProjectLaunchTargetEditorFunctionalityTest.java (7)
42-47: LGTM - Clean test setup refactoring.The migration to a centralized
Fixture.createProject()method improves consistency with the broader test suite refactoring.
49-53: LGTM - Improved teardown naming.The rename to
tearDown()is more idiomatic and aligns with standard JUnit conventions.
55-62: LGTM - Improved test method naming.The rename to a behavior-driven "should..." convention makes the test intent clearer and aligns with modern testing best practices.
64-72: LGTM - Clearer test method naming.The rename improves readability and follows behavior-driven naming conventions.
195-200: Package-private visibility is intentional and necessary.The
cleanupEnvironment()method is shared across multiple test classes (NewEspressifIDFProjectPartitionTableEditorTest, NewEspressifIDFProjectSDKconfigTest, NewEspressifIDFProjectSBOMTest, NewEspressifIDFProjectClangFilesTest, IDFProjectJTAGFlashTest, NewEspressifIDFProjectFlashProcessTest), justifying the package-private visibility change.
80-85: Package-private visibility is intentional and justified.The
loadEnv()method is accessed by multiple test classes in the same package (NewEspressifIDFProjectSDKconfigTest,NewEspressifIDFProjectPartitionTableEditorTest,NewEspressifIDFProjectFlashProcessTest,NewEspressifIDFProjectClangFilesTest,NewEspressifIDFProjectTest,LaunchBarCDTConfigurationsTest,IDFProjectJTAGFlashTest, andNewEspressifIDFProjectSBOMTest), confirming that the visibility change fromprivateto package-private is intentional for shared test initialization.
87-90: No changes required. The"EspressIf"category name is correct and consistent with the project wizard UI. This spelling is used consistently across all Espressif IDF test files.Likely an incorrect or invalid review comment.
tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/executable/cases/project/NewEspressifIDFProjectClangFilesTest.java (10)
45-47: LGTM!The introduction of explicit project name constants for multi-project orchestration improves test clarity and maintainability.
48-54: LGTM!Class-level project creation aligns with the multi-project test strategy and the ordered test execution via
@FixMethodOrder.
56-60: LGTM!Migrating to
@AfterClassis appropriate for the ordered, stateful test design. The cleanup waits for async operations before closing and deleting projects.
62-73: LGTM!The test method is well-structured with clear BDD-style naming and proper project parameterization.
75-85: LGTM!The deletion and recreation workflow is properly tested with clear step ordering.
128-132: LGTM!The
createProjectmethod is properly parameterized to support multi-project orchestration and delegates to the sharedProjectTestOperations.setupProjectutility.
121-126: LGTM!Clean refactoring with proper method delegation.
158-198: LGTM!The fixture methods are consistently parameterized with
projectName, enabling clean multi-project test orchestration.
215-228: LGTM!The distinction between
thenCleanProjectClangdFileContentChecked(relative path) andthenClangdFileContentChecked(absolute path after build) correctly reflects the different project states being validated.
394-399: LGTM!The cleanup sequence properly waits for async operations before closing and deleting projects, ensuring clean teardown.
releng/com.espressif.idf.target/com.espressif.idf.target.target (1)
104-112: LGTM. The LSP4E 0.29.3 upgrade is structurally correct and all dependencies are verified: the repository and specified versions are accessible, compatible with Eclipse 2025-09, and have no known security vulnerabilities. The pinned versions fororg.eclipse.lsp4eandorg.eclipse.lsp4e.debugprovide good build reproducibility.docs/zh_CN/additionalfeatures/wokwisimulator.rst (1)
1-15: LGTM! Well-formatted Chinese documentation.The Chinese documentation is properly formatted with correct reStructuredText syntax, clear step-by-step instructions, and appropriate use of inline code literals and external links. This localization effort aligns well with the PR's objectives.
docs/zh_CN/additionalfeatures/dfu.rst (1)
1-46: LGTM! Comprehensive Chinese DFU documentation.The Chinese documentation for DFU is well-structured with proper reStructuredText formatting, including a correctly formatted pin mapping table, clear requirements, and step-by-step instructions. The localization is complete and aligns with the PR's objectives.
bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/SetupToolsInIde.java (1)
46-46: LGTM! Import added for LSP integration.The import for
LspServiceis appropriately added to support the new Clangd preferences configuration functionality.docs/zh_CN/additionalfeatures/clangd_cdt_support.rst (1)
1-95: LGTM! Comprehensive Chinese clangd/CDT documentation.The Chinese documentation provides complete coverage of the LSP-based C/C++ editor feature, including:
- Clear explanation of why CDT indexer was replaced with LSP
- Configuration steps with proper code examples (YAML and CMake)
- Troubleshooting guidance for common issues
- Proper reStructuredText formatting throughout
This documentation aligns well with the code changes in
SetupToolsInIde.javathat integrate LSP service configuration.docs/en/additionalfeatures/wokwisimulator.rst (1)
4-15: LGTM! Documentation formatting improvements.The formatting enhancements improve readability and consistency:
- Added Chinese translation link for better documentation navigation
- Applied inline code formatting to UI elements, configuration values, and technical terms
- Consistent use of code literals throughout the steps
These changes align well with the broader documentation localization and quality improvements in this PR.
docs/en/additionalfeatures/clangd_cdt_support.rst (3)
6-14: Strong documentation structure and clarity improvements.The translation link is properly added, and the introduction is well-refined to clarify LSP support and explain the C++ standard progression (C++14 → C++20/C++23) with corresponding GCC versions. The wording on line 14 is appropriately toned down to lowercase.
88-95: CDT Indexer disable section is clear and consistent.The instructions are properly formatted with numbered steps and code formatting for UI navigation paths. The image reference is included, and the default behavior (CDT Indexer disabled by default in 3.0.0+) is clearly documented.
51-84: New error resolution section adds valuable troubleshooting guidance.The section on fixing "Unknown Argument" errors is well-documented with clear problem statement and error examples, numbered steps with actionable instructions, proper code block formatting for both Python and CMake, and links to the actual scripts in the repository. The structure is logical and would help developers quickly resolve clangd configuration issues.
docs/en/additionalfeatures/dfu.rst (1)
1-46: Well-executed documentation improvements.The changes to this file are solid: the translation link marker is properly added, the ASCII table is correctly converted to RST list-table format with proper indentation and headers, and link formatting is updated to standard RST style. Content clarifications (e.g., ensuring device is in download mode before running Zadig) enhance the instructions. All external URLs referenced in the document are accessible and functional.
| monitor.setTaskName("Applying Clangd Preferences"); //$NON-NLS-1$ | ||
| log("Applying Clangd Preferences", IStatus.INFO); //$NON-NLS-1$ | ||
| var lspService = new LspService(); | ||
| lspService.updateClangdPath(); | ||
| lspService.updateQueryDriver(); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find LspService class definition
find . -type f -name "LspService.java" | head -20Repository: espressif/idf-eclipse-plugin
Length of output: 152
🏁 Script executed:
#!/bin/bash
# Search for LspService class across the codebase
rg "class LspService" -A 30Repository: espressif/idf-eclipse-plugin
Length of output: 3510
🏁 Script executed:
#!/bin/bash
# Look at the full context of lines around 154-158 in SetupToolsInIde.java
find . -type f -name "SetupToolsInIde.java" | head -5Repository: espressif/idf-eclipse-plugin
Length of output: 158
🏁 Script executed:
#!/bin/bash
# Get the full LspService implementation to check exception signatures
cat ./bundles/com.espressif.idf.core/src/com/espressif/idf/core/util/LspService.javaRepository: espressif/idf-eclipse-plugin
Length of output: 3622
🏁 Script executed:
#!/bin/bash
# Get context around lines 154-158 in SetupToolsInIde.java
sed -n '100,170p' ./bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/SetupToolsInIde.javaRepository: espressif/idf-eclipse-plugin
Length of output: 2787
Add exception handling for LSP service configuration.
The LspService constructor and methods can throw unchecked exceptions from Eclipse APIs (e.g., IllegalStateException, NullPointerException) that are not caught by the outer try-catch block (which only catches IOException). If LSP configuration fails, the entire tool setup process will abort.
Since LSP configuration appears to be an optional feature (placed as the final step and other methods gracefully handle missing executables), wrap these calls in a separate try-catch to log failures without aborting setup:
Suggested fix
monitor.setTaskName("Applying Clangd Preferences"); //$NON-NLS-1$
log("Applying Clangd Preferences", IStatus.INFO); //$NON-NLS-1$
-var lspService = new LspService();
-lspService.updateClangdPath();
-lspService.updateQueryDriver();
+try
+{
+ var lspService = new LspService();
+ lspService.updateClangdPath();
+ lspService.updateQueryDriver();
+}
+catch (Exception e)
+{
+ Logger.log(e);
+ log("Failed to apply Clangd preferences: " + e.getMessage(), IStatus.WARNING); //$NON-NLS-1$
+}📝 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.
| monitor.setTaskName("Applying Clangd Preferences"); //$NON-NLS-1$ | |
| log("Applying Clangd Preferences", IStatus.INFO); //$NON-NLS-1$ | |
| var lspService = new LspService(); | |
| lspService.updateClangdPath(); | |
| lspService.updateQueryDriver(); | |
| monitor.setTaskName("Applying Clangd Preferences"); //$NON-NLS-1$ | |
| log("Applying Clangd Preferences", IStatus.INFO); //$NON-NLS-1$ | |
| try | |
| { | |
| var lspService = new LspService(); | |
| lspService.updateClangdPath(); | |
| lspService.updateQueryDriver(); | |
| } | |
| catch (Exception e) | |
| { | |
| Logger.log(e); | |
| log("Failed to apply Clangd preferences: " + e.getMessage(), IStatus.WARNING); //$NON-NLS-1$ | |
| } |
🤖 Prompt for AI Agents
In
@bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/SetupToolsInIde.java
around lines 154 - 158, Wrap the LSP configuration block that constructs
LspService and calls updateClangdPath() and updateQueryDriver() in its own
try-catch so runtime/unchecked exceptions do not abort setup; specifically
surround the code that creates new LspService() and calls
lspService.updateClangdPath() and lspService.updateQueryDriver() with a try {
... } catch (RuntimeException | Exception e) { log("Applying Clangd Preferences
failed: " + e.getMessage(), IStatus.WARNING); /* optionally log stack trace */ }
(preserve monitor.setTaskName and the original log call on success) so failures
are logged and setup continues.
| By default, the esp-clang toolchain is installed as a part of the ESP-IDF tools installation process, and clangd ``Path`` is configured in the preferences. | ||
|
|
||
| The **Drivers** path and **--compile-commands-dir** path will be configured based on the selected target (esp32, esp32c6 etc.) and the project you're building. | ||
| The ``Drivers`` path and ``--compile-commands-dir`` path will be configured based on the selected target (esp32, esp32c6, etc.) and the project being built. | ||
|
|
||
| However, if there are any issues in configuration, this can be configured in the following way: | ||
|
|
||
| 1. Go to `Window` > `Preferences` > `C/C++` > `Editor(LSP)` | ||
| 2. Navigate to `clangd` node | ||
| 3. Provide `Drivers` path as shown in the screenshot. | ||
| 4. Set `--compile-commands-dir=/project/build` in the additional argument section. | ||
| 5. Click on `Apply and Close`. | ||
| 1. Go to ``Window`` > ``Preferences`` > ``C/C++`` > ``Editor(LSP)``. | ||
| 2. Navigate to ``clangd`` node. | ||
| 3. Provide ``Drivers`` path as shown in the screenshot. | ||
| 4. Set ``--compile-commands-dir=/project/build`` in the additional argument section. | ||
| 5. Click on ``Apply and Close``. | ||
|
|
||
| .. image:: ../../../media/clangd/clangd_config.png | ||
| .. image:: ../../../media/clangd/clangd_config.png | ||
|
|
||
| By default, when you create a new project, a `.clangd` configuration file is created with the following arguments. | ||
| By default, when you create a new project, a ``.clangd`` configuration file is generated automatically. | ||
|
|
||
| However, if you are dealing with an existing project, please create a `.clangd` file at the root of the project and add the following content. | ||
| If you are migrating a project from an older version, you need to create a ``.clangd`` file manually in the project root and add the following content: | ||
|
|
||
| .. code-block:: yaml | ||
| CompileFlags: | ||
| CompilationDatabase: build | ||
| Remove: [-m*, -f*] | ||
| CompilationDatabase: build | ||
| Remove: [-m*, -f*] | ||
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify that image files referenced in clangd_cdt_support.rst exist in the repo
image_files=(
"media/clangd/cdtlsp_updatesite.png"
"media/clangd/clangd_config.png"
"media/clangd/cdt_indexer_disable.png"
)
# Search for these files in the repository
for image in "${image_files[@]}"; do
result=$(fd -t f "$image" 2>/dev/null | head -1)
if [ -z "$result" ]; then
echo "✗ Not found: $image"
else
echo "✓ Found: $result"
fi
doneRepository: espressif/idf-eclipse-plugin
Length of output: 213
🏁 Script executed:
#!/bin/bash
# First, let's find all PNG files related to clangd
echo "=== Searching for clangd-related PNG files ==="
fd -t f ".*clangd.*\.png"
echo ""
echo "=== Checking if media directory exists ==="
fd -t d "media" --max-depth 3
echo ""
echo "=== Listing all files in any clangd directory ==="
fd -t d "clangd" -exec find {} -type f \;
echo ""
echo "=== Checking docs structure ==="
ls -la docs/en/additionalfeatures/ 2>/dev/null | head -20Repository: espressif/idf-eclipse-plugin
Length of output: 2382
🏁 Script executed:
#!/bin/bash
# Check the actual image directive in the RST file
cat -n docs/en/additionalfeatures/clangd_cdt_support.rst | grep -A 2 -B 2 "image::"Repository: espressif/idf-eclipse-plugin
Length of output: 738
Image references contain broken paths that must be fixed.
The documentation includes three image directives, but verification reveals:
../../../media/clangd/cdtlsp_updatesite.png(line 22) is incorrect; the file ismedia/clangd_updatesite.png(not in a clangd subdirectory)../../../media/clangd/clangd_config.png(line 39) is correct../../../media/clangd/cdt_indexer_disable.png(line 95) does not exist in the repository
Update the image paths on lines 22 and 95 to reference the correct files, or add the missing image for the indexer configuration.
🤖 Prompt for AI Agents
In @docs/en/additionalfeatures/clangd_cdt_support.rst around lines 27 - 50,
Update the broken image directives in the .rst: replace the .. image::
../../../media/clangd/cdtlsp_updatesite.png reference with .. image::
media/clangd_updatesite.png (the actual file name), ensure the existing ..
image:: ../../../media/clangd/clangd_config.png is left as-is, and either
correct the final .. image:: ../../../media/clangd/cdt_indexer_disable.png to
point to the real image filename in the media directory or add the missing
cdt_indexer_disable.png into the media folder and reference it from the
document; make these changes on the corresponding image directive lines so the
.. image:: directives point to valid files.
| public void shouldMatchExpectedClangdArgumentsAfterBuildingProjects() throws Exception | ||
| { | ||
| Fixture.givenNewProjectIsCreated("NewProjectClangFilesTest6"); | ||
| Fixture.thenClangdPathUpdateOnSelectedTarget(); | ||
| Fixture.thenQueryDriversUpdateOnSelectedTarget(); | ||
| Fixture.whenProjectIsBuiltUsingContextMenu(CLEAN_PROJECT2); | ||
| Fixture.thenCheckClangdArgumentAfterProjectBuilt(CLEAN_PROJECT2); | ||
| Fixture.whenProjectIsBuiltUsingContextMenu(CLEAN_PROJECT1); | ||
| Fixture.thenCheckClangdArgumentAfterProjectBuilt(CLEAN_PROJECT1); | ||
| Fixture.thenClangdDriversUpdateOnSelectedTarget(); | ||
| } |
There was a problem hiding this comment.
Missing @Test annotation - test will not execute.
This test method lacks the @Test annotation, so it will be silently skipped by the JUnit runner. The clangd argument verification logic will never run.
🐛 Proposed fix
+ @Test
public void shouldMatchExpectedClangdArgumentsAfterBuildingProjects() 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 101 - 108, Add the missing @Test annotation to the method
shouldMatchExpectedClangdArgumentsAfterBuildingProjects so the JUnit runner
executes it; place @Test immediately above the method declaration and ensure the
matching Test import (e.g., org.junit.Test or org.junit.jupiter.api.Test) is
present in the file, adjusting other tests in this package to match the
project's JUnit version if needed.
| private static void thenClangdPathUpdateOnSelectedTarget() throws Exception | ||
| { | ||
| whenOpenClangdPreferences(); | ||
| thenCompareActualClangdPathWithExpected("NewProjectClangFilesTest6"); | ||
| } | ||
|
|
||
| private static void thenQueryDriversUpdateOnSelectedTarget() throws Exception | ||
| { | ||
| thenCompareActualQueryDriversWithExpected("NewProjectClangFilesTest6"); | ||
| thenCompareActualClangdDriversWithExpected(); | ||
| closePreferencesDialog(); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -n "thenClangdPathUpdateOnSelectedTarget" --type javaRepository: espressif/idf-eclipse-plugin
Length of output: 282
🏁 Script executed:
rg -n "thenClangdDriversUpdateOnSelectedTarget" --type javaRepository: espressif/idf-eclipse-plugin
Length of output: 471
🏁 Script executed:
cat -n "tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/executable/cases/project/NewEspressifIDFProjectClangFilesTest.java" | sed -n '140,170p'Repository: espressif/idf-eclipse-plugin
Length of output: 1342
🏁 Script executed:
cat -n "tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/executable/cases/project/NewEspressifIDFProjectClangFilesTest.java" | sed -n '121,135p'Repository: espressif/idf-eclipse-plugin
Length of output: 707
Remove unused method thenClangdPathUpdateOnSelectedTarget.
This method is never called and duplicates the implementation of thenClangdDriversUpdateOnSelectedTarget (which is actively used at line 107). It should be removed.
🤖 Prompt for AI Agents
In
@tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/executable/cases/project/NewEspressifIDFProjectClangFilesTest.java
around lines 151 - 156, Remove the unused duplicate method
thenClangdPathUpdateOnSelectedTarget(): locate its declaration and delete the
entire private static void thenClangdPathUpdateOnSelectedTarget() method body
since its behavior is duplicated by thenClangdDriversUpdateOnSelectedTarget,
which is the method actually used; ensure no other references to
thenClangdPathUpdateOnSelectedTarget remain (if any, replace them with
thenClangdDriversUpdateOnSelectedTarget).
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/executable/cases/project/NewEspressifIDFProjectClangFilesTest.java (1)
362-368: Strengthen the clangd path assertion.The assertion
actualClangdPath.contains("bin")is too permissive and could match unintended paths (e.g.,/binary/tools,/rubin/path, or any path containing "bin" as a substring).Consider a more specific check to avoid false positives:
🔍 Suggested improvements
Option 1: Check if path ends with expected directory structure:
- String expectedClangdPath = "bin"; - assertTrue(actualClangdPath.contains(expectedClangdPath)); + assertTrue("Expected clangd path to end with 'bin', but got: " + actualClangdPath, + actualClangdPath.endsWith("bin") || actualClangdPath.endsWith("bin/"));Option 2: Use a more specific pattern match:
- String expectedClangdPath = "bin"; - assertTrue(actualClangdPath.contains(expectedClangdPath)); + assertTrue("Expected clangd path to contain '/bin' directory, but got: " + actualClangdPath, + actualClangdPath.matches(".*[\\\\/]bin[\\\\/]?$"));
🧹 Nitpick comments (2)
tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/executable/cases/project/NewEspressifIDFProjectClangFilesTest.java (2)
45-46: Consider more descriptive constant names.The
CLEAN_prefix suggests these projects remain unmodified, but tests actually modify them (e.g.,shouldApplyClangFormatSettingsWhenAutoSaveIsEnabledmodifies Project2,shouldRecreateClangdFileAfterDeletionAndVerifyContentmodifies Project1). Consider names likeSHARED_PROJECT1or simplyPROJECT1to avoid implying immutability.
152-157: Remove unused method.The method
thenClangdPathUpdateOnSelectedTarget()is never called in the test class and appears to be dead code. Consider removing it to reduce maintenance burden.🧹 Proposed cleanup
- private static void thenClangdPathUpdateOnSelectedTarget() throws Exception - { - whenOpenClangdPreferences(); - thenCompareActualClangdDriversWithExpected(); - closePreferencesDialog(); - } -
📜 Review details
Configuration used: defaults
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
🧰 Additional context used
🧬 Code graph analysis (1)
tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/executable/cases/project/NewEspressifIDFProjectClangFilesTest.java (3)
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/operations/EnvSetupOperations.java (1)
EnvSetupOperations(16-89)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). (3)
- GitHub Check: build_linux
- GitHub Check: build_windows
- GitHub Check: build_macos
🔇 Additional comments (1)
tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/executable/cases/project/NewEspressifIDFProjectClangFilesTest.java (1)
63-109: Verify test interdependencies are acceptable.The tests share project state and rely on alphabetical execution order (
@FixMethodOrder):
shouldApply...(runs 1st) modifies Project2's clang-format and main.cshouldHave...(runs 2nd) verifies Project1's clean stateshouldMatch...(runs 3rd) builds both projects (Project2 with modifications from step 1)shouldRecreate...(runs 4th) expects Project1 to have build directory (from step 3)This creates test interdependencies where:
- Tests cannot be run individually or in a different order
- Earlier test failures may cause later tests to fail
- Debugging requires understanding the entire test sequence
While the AI summary indicates this aligns with the broader PR pattern of class-level lifecycle management for performance, confirm this trade-off against test isolation is acceptable for your test suite architecture.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/executable/cases/project/NewEspressifIDFProjectClangFilesTest.java (2)
349-354: Consider usingassertEqualsfor better failure messages.Using
assertTrue(expectedClangdPath.equals(actualClangdPath))loses diagnostic information on failure.assertEqualswould show both expected and actual values in the failure message.Suggested improvement
private static void thenCompareActualClangdArgumentWithExpected(String projectName) throws IOException { SWTBotShell prefrencesShell = bot.shell("Preferences"); String actualClangdPath = prefrencesShell.bot().textWithLabel("Additional").getText(); String expectedClangdPath = getExpectedBuildFolderPATHforClangdAdditionalArgument(projectName); - assertTrue(expectedClangdPath.equals(actualClangdPath)); + assertEquals(expectedClangdPath, actualClangdPath); }This would require adding
import static org.junit.Assert.assertEquals;at the top.
390-394: Consider parameterizing the project name for consistency.This method directly references
CLEAN_PROJECT1instead of accepting a parameter like other Fixture methods. While functional, parameterizing it would improve consistency and reusability.Suggested improvement
-private static void whenSelectProjectInLaunchConfig() throws Exception +private static void whenSelectProjectInLaunchConfig(String projectName) throws Exception { LaunchBarConfigSelector configSelector = new LaunchBarConfigSelector(bot); - configSelector.select(CLEAN_PROJECT1); + configSelector.select(projectName); }Then update the call site at line 107:
-Fixture.whenSelectProjectInLaunchConfig(); +Fixture.whenSelectProjectInLaunchConfig(CLEAN_PROJECT1);
📜 Review details
Configuration used: defaults
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
⏰ 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_linux
- GitHub Check: build_macos
🔇 Additional comments (8)
tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/executable/cases/project/NewEspressifIDFProjectClangFilesTest.java (8)
46-48: LGTM!The project identifier constants are well-defined and support the multi-project test coordination. The naming is clear and consistent.
49-55: Class-level setup looks correct, but test ordering has implicit dependencies.The multi-project setup in
@BeforeClassis appropriate for the shared state model. However, be aware that with@FixMethodOrder(NAME_ASCENDING), the execution order is:
shouldApplyClangFormatSettingsWhenAutoSaveIsEnabled(modifies Project2's.clang-format)shouldHaveClangFilesPresentAndContentCorrectForNewProject(checks Project1)shouldMatchExpectedClangdArgumentsAfterBuildingProjects(builds both projects)shouldRecreateClangdFileAfterDeletionAndVerifyContent(deletes/recreates on Project1)This ordering appears intentional but could be fragile if test method names change in the future.
57-61: LGTM!The switch to
@AfterClassis consistent with the class-level@BeforeClasssetup. Note that if a test fails mid-execution, subsequent tests will continue with potentially corrupt state, but this is an acceptable trade-off for test execution speed.
63-74: LGTM!The test method is well-structured with clear BDD-style naming. The flow correctly verifies file presence, opens files, checks content, and closes editors.
76-86: LGTM!The test correctly verifies the clangd file recreation workflow. The use of
thenClangdFileContentChecked(CLEAN_PROJECT1)(which includes the full build path) is appropriate here since this test runs aftershouldMatchExpectedClangdArgumentsAfterBuildingProjectshas already built the projects.
88-100: LGTM!Good separation of concerns by using
CLEAN_PROJECT2for this test, which modifies the.clang-formatsettings. This isolation prevents interference with other tests that useCLEAN_PROJECT1.
102-111: LGTM!The test correctly builds both projects and verifies the clangd arguments are updated with the expected build paths. The launch configuration selection between project builds ensures proper context switching.
396-401: LGTM!The renamed
cleanupEnvironment()method is more descriptive and properly handles test cleanup by waiting for operations to finish, closing all projects, and deleting them.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
@tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/executable/cases/project/NewEspressifIDFProjectClangFilesTest.java:
- Around line 359-365: In thenCompareActualClangdDriversWithExpected(), the
assertEquals parameters are reversed causing misleading messages on failure;
swap the arguments so the expected value (expectedClangdPath from
IDFUtil.findCommandFromBuildEnvPath(ILSPConstants.CLANGD_EXECUTABLE)) is the
first parameter and the actual value (actualClangdPath from
prefrencesShell.bot().textWithLabel("Path").getText()) is the second.
🧹 Nitpick comments (3)
tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/executable/cases/project/NewEspressifIDFProjectClangFilesTest.java (3)
90-102: Missing preference restoration after test.
setupAutoSave()enables "Format source code" in preferences, but this setting is never restored to its original state. This could affect subsequent tests or leave the test environment in a dirty state.Consider adding a cleanup step to restore the preference:
♻️ Suggested approach
Add a method to restore the preference and call it in cleanup:
private static void restoreAutoSavePreference() throws Exception { bot.menu("Window").menu("Preferences...").click(); SWTBotShell prefrencesShell = bot.shell("Preferences"); prefrencesShell.bot().tree().getTreeItem("C/C++").select(); prefrencesShell.bot().tree().getTreeItem("C/C++").expand(); prefrencesShell.bot().tree().getTreeItem("C/C++").getNode("Editor (LSP)").select(); prefrencesShell.bot().tree().getTreeItem("C/C++").getNode("Editor (LSP)").expand(); prefrencesShell.bot().tree().getTreeItem("C/C++").getNode("Editor (LSP)").getNode("Save Actions").select(); // Uncheck if currently checked if (prefrencesShell.bot().checkBox("Format source code").isChecked()) { prefrencesShell.bot().checkBox("Format source code").click(); } prefrencesShell.bot().button("Apply and Close").click(); }
392-396: Consider parameterizing the project name for consistency.This method hardcodes
CLEAN_PROJECT1while similar methods likewhenProjectIsBuiltUsingContextMenuacceptprojectNameas a parameter. For consistency and reusability:♻️ Suggested refactor
-private static void whenSelectProjectInLaunchConfig() throws Exception +private static void whenSelectProjectInLaunchConfig(String projectName) throws Exception { LaunchBarConfigSelector configSelector = new LaunchBarConfigSelector(bot); - configSelector.select(CLEAN_PROJECT1); + configSelector.select(projectName); }And update the call site accordingly:
-Fixture.whenSelectProjectInLaunchConfig(); +Fixture.whenSelectProjectInLaunchConfig(CLEAN_PROJECT1);
278-293: Consider using explicit wait instead of hard-coded sleep.
bot.sleep(1000)at line 280 is a brittle timing mechanism. If the formatting operation takes longer than expected, the test could fail intermittently.Consider replacing with a condition-based wait that polls for the expected content, or increase the timeout if flaky tests are observed.
📜 Review details
Configuration used: defaults
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
⏰ 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_linux
- GitHub Check: build_macos
- GitHub Check: build_windows
🔇 Additional comments (5)
tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/executable/cases/project/NewEspressifIDFProjectClangFilesTest.java (5)
43-57: Class-level setup with ordered tests introduces test interdependency.Using
@FixMethodOrder(MethodSorters.NAME_ASCENDING)with@BeforeClass/@AfterClassmeans tests share mutable state across the class. The test methods (all starting withshould...) will execute in alphabetical order:
shouldApplyClangFormatSettingsWhenAutoSaveIsEnabledshouldHaveClangFilesPresentAndContentCorrectForNewProjectshouldMatchExpectedClangdArgumentsAfterBuildingProjectsshouldRecreateClangdFileAfterDeletionAndVerifyContentThis ordering may not align with the intended test flow. For example,
shouldRecreateClangdFileAfterDeletionAndVerifyContentruns last but logically should run before build tests. Consider either:
- Renaming tests with numeric prefixes (e.g.,
a_shouldHave...,b_shouldRecreate...) to control order explicitly- Using
@Orderannotation if migrating to JUnit 5
59-63: LGTM!The class-level teardown with
cleanupEnvironment()is appropriate for SWTBot UI tests where setup costs are high. The cleanup properly waits for operations and cleans up all projects.
351-357: LGTM!Assertion argument order is correct here (
expected,actual).
115-138: LGTM!The Fixture class structure is clean with clear separation of setup operations. The
loadEnvandcreateProjectmethods properly initialize the test environment.
398-403: LGTM!The cleanup method properly waits for async operations to complete before closing and deleting projects, which is the correct pattern for SWTBot cleanup.
sigmaaa
force-pushed
the
cherry-pick-from-v4.0.0
branch
from
8ca1677 to
9df7cc3
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
@tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/executable/cases/project/NewEspressifIDFProjectClangFilesTest.java:
- Around line 359-365: In thenCompareActualClangdDriversWithExpected(), the
assertEquals call uses reversed arguments; change assertEquals(actualClangdPath,
expectedClangdPath) to assertEquals(expectedClangdPath, actualClangdPath) so the
JUnit failure message reports expected vs actual in the correct order (use the
variables expectedClangdPath and actualClangdPath in that order).
🧹 Nitpick comments (2)
tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/executable/cases/project/NewEspressifIDFProjectClangFilesTest.java (2)
104-113: Consider parameterizingwhenSelectProjectInLaunchConfigfor consistency.The method
whenSelectProjectInLaunchConfig()hardcodesCLEAN_PROJECT1internally (line 395), which is inconsistent with other fixture methods that acceptprojectNameas a parameter. This reduces flexibility and readability.♻️ Suggested refactor
@Test public void shouldMatchExpectedClangdArgumentsAfterBuildingProjects() throws Exception { Fixture.whenProjectIsBuiltUsingContextMenu(CLEAN_PROJECT2); Fixture.thenCheckClangdArgumentAfterProjectBuilt(CLEAN_PROJECT2); - Fixture.whenSelectProjectInLaunchConfig(); + Fixture.whenSelectProjectInLaunchConfig(CLEAN_PROJECT1); Fixture.whenProjectIsBuiltUsingContextMenu(CLEAN_PROJECT1); Fixture.thenCheckClangdArgumentAfterProjectBuilt(CLEAN_PROJECT1); Fixture.thenClangdDriversUpdateOnSelectedTarget(); }And update the fixture method:
-private static void whenSelectProjectInLaunchConfig() throws Exception +private static void whenSelectProjectInLaunchConfig(String projectName) throws Exception { LaunchBarConfigSelector configSelector = new LaunchBarConfigSelector(bot); - configSelector.select(CLEAN_PROJECT1); + configSelector.select(projectName); }
156-177: Consider using more specific assertions for better failure messages.The current assertion patterns work but could provide clearer failure diagnostics:
assertTrue(x != null)→assertNotNull(x)— provides "expected non-null" messageassertTrue(!condition)→assertFalse(condition)— clearer intent♻️ Suggested improvements
private static void thenClangdFileIsPresent(String projectName) throws IOException { - assertTrue(bot.tree().getTreeItem(projectName).getNode(".clangd") != null); + assertNotNull(bot.tree().getTreeItem(projectName).getNode(".clangd")); } private static void thenClangFormatFileIsPresent(String projectName) throws IOException { - assertTrue(bot.tree().getTreeItem(projectName).getNode(".clang-format") != null); + assertNotNull(bot.tree().getTreeItem(projectName).getNode(".clang-format")); } ... private static void thenClangdFileIsAbsent(String projectName) throws IOException { - assertTrue(!bot.tree().getTreeItem(projectName).getNodes().contains(".clangd")); + assertFalse(bot.tree().getTreeItem(projectName).getNodes().contains(".clangd")); }This requires adding imports:
import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertNotNull;
📜 Review details
Configuration used: defaults
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
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: build_windows
- GitHub Check: build_linux
- GitHub Check: build_macos
🔇 Additional comments (4)
tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/executable/cases/project/NewEspressifIDFProjectClangFilesTest.java (4)
51-63: Class-level lifecycle is appropriate for this integration test pattern.The migration from
@Afterto@AfterClasswith@FixMethodOrder(MethodSorters.NAME_ASCENDING)is a valid approach for reducing setup overhead in UI integration tests. The trade-off is reduced test isolation—if an earlier test fails, subsequent tests may be affected. This is acceptable given the sequential nature of UI tests and the overhead of project creation.
65-76: LGTM!The test correctly verifies presence and content of both
.clangdand.clang-formatfiles for a new project. The use ofthenCleanProjectClangdFileContentChecked()(relative path) vsthenClangdFileContentChecked(projectName)(absolute path) appropriately distinguishes between initial project state and recreated file state.
78-88: LGTM!The test properly validates the file recreation workflow: delete, verify absence, recreate via context menu, verify presence, and validate content with the expected absolute build path.
90-102: LGTM!Good test isolation by using
CLEAN_PROJECT2for clang-format testing, avoiding interference withCLEAN_PROJECT1's file state from previous tests.
| 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 expectedClangdPath = IDFUtil.findCommandFromBuildEnvPath(ILSPConstants.CLANGD_EXECUTABLE); | ||
| assertEquals(actualClangdPath, expectedClangdPath); | ||
| } |
There was a problem hiding this comment.
Reversed assertEquals arguments will produce misleading failure messages.
Line 364 has assertEquals(actualClangdPath, expectedClangdPath) but JUnit's convention is assertEquals(expected, actual). This won't affect whether the test passes or fails, but if it fails, the error message will show the values in the wrong order, making debugging harder.
🐛 Proposed fix
private static void thenCompareActualClangdDriversWithExpected() throws IOException
{
SWTBotShell prefrencesShell = bot.shell("Preferences");
String actualClangdPath = prefrencesShell.bot().textWithLabel("Path").getText();
String expectedClangdPath = IDFUtil.findCommandFromBuildEnvPath(ILSPConstants.CLANGD_EXECUTABLE);
- assertEquals(actualClangdPath, expectedClangdPath);
+ assertEquals(expectedClangdPath, actualClangdPath);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| private static void thenCompareActualClangdDriversWithExpected() throws IOException | |
| { | |
| SWTBotShell prefrencesShell = bot.shell("Preferences"); | |
| String actualClangdPath = prefrencesShell.bot().textWithLabel("Path").getText(); | |
| String expectedClangdPath = "bin"; | |
| assertTrue(actualClangdPath.contains(expectedClangdPath)); | |
| String expectedClangdPath = IDFUtil.findCommandFromBuildEnvPath(ILSPConstants.CLANGD_EXECUTABLE); | |
| assertEquals(actualClangdPath, expectedClangdPath); | |
| } | |
| private static void thenCompareActualClangdDriversWithExpected() throws IOException | |
| { | |
| SWTBotShell prefrencesShell = bot.shell("Preferences"); | |
| String actualClangdPath = prefrencesShell.bot().textWithLabel("Path").getText(); | |
| String expectedClangdPath = IDFUtil.findCommandFromBuildEnvPath(ILSPConstants.CLANGD_EXECUTABLE); | |
| assertEquals(expectedClangdPath, actualClangdPath); | |
| } |
🤖 Prompt for AI Agents
In
@tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/executable/cases/project/NewEspressifIDFProjectClangFilesTest.java
around lines 359 - 365, In thenCompareActualClangdDriversWithExpected(), the
assertEquals call uses reversed arguments; change assertEquals(actualClangdPath,
expectedClangdPath) to assertEquals(expectedClangdPath, actualClangdPath) so the
JUnit failure message reports expected vs actual in the correct order (use the
variables expectedClangdPath and actualClangdPath in that order).
AndriiFilippov
left a comment
AndriiFilippov
left a comment
There was a problem hiding this comment.
@sigmaaa hi !
LGTM 👍
Description
I've noticed that some commits from the 4.0 are missing on the master after the merge. In this PR I cherry-picked the missing commits and resolved conflicts
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
New Features
Documentation
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.