SWTBot test case: Clangd / Clang-Format Files Test #1131
Conversation
WalkthroughThe changes introduce a new automated test class for verifying Clangd and Clang-Format integration in Espressif IDF projects, expand utility methods for file operations and text comparison in editors, and generalize tab-waiting utilities. Existing tests are updated to use the new generic tab-waiting method, and additional waits and refresh actions are added to improve test reliability. The Java execution environment requirement is updated, and editor preference selection is refined. Changes
Sequence Diagram(s)sequenceDiagram
participant TestClass as NewEspressifIDFProjectClangFilesTest
participant Fixture as Fixture (Utility)
participant SWTBot as SWTWorkbenchBot
participant ProjectExplorer as Project Explorer
participant Editor as Editor
TestClass->>Fixture: beforeTestClass()
Fixture->>SWTBot: setUpEnvironment()
Fixture->>SWTBot: deleteExistingProjects()
TestClass->>Fixture: givenNewProjectIsCreatedThenTestClangFilesPresenceAndContentForNewProject()
Fixture->>SWTBot: createNewProject()
Fixture->>ProjectExplorer: check .clangd and .clang-format exist
Fixture->>Editor: open files and verify content
Editor-->>Fixture: return file content
Fixture->>Editor: close editors
TestClass->>Fixture: givenNewProjectIsCreatedWhenClangdFileIsDeletedThenTestClangdFileCreatedUsingContextMenuThenClangdFileContentChecked()
Fixture->>ProjectExplorer: delete .clangd
Fixture->>ProjectExplorer: recreate .clangd via context menu
Fixture->>Editor: verify .clangd content
TestClass->>Fixture: givenNewProjectIsCreatedThenTestClangFormatFileSettingsAreBeingApplied()
Fixture->>Editor: edit .clang-format settings
Fixture->>Editor: save file
Fixture->>Editor: open main.c, add spaces
Fixture->>Editor: trigger formatting
Editor-->>Fixture: return formatted content
Fixture->>TestClass: assert formatting applied
Poem
Note ⚡️ AI Code Reviews for VS Code, Cursor, WindsurfCodeRabbit now has a plugin for VS Code, Cursor and Windsurf. This brings AI code reviews directly in the code editor. Each commit is reviewed immediately, finding bugs before the PR is raised. Seamless context handoff to your AI code agent ensures that you can easily incorporate review feedback. Note ⚡️ Faster reviews with cachingCodeRabbit now supports caching for code and dependencies, helping speed up reviews. This means quicker feedback, reduced wait times, and a smoother review experience overall. Cached data is encrypted and stored securely. This feature will be automatically enabled for all accounts on May 16th. To opt out, configure 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (1)
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![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
🧹 Nitpick comments (13)
tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/operations/ProjectTestOperations.java (1)
273-277: Consider adding input validation.While the implementation is correct, consider adding null checks to prevent NullPointerException.
private static String normalizeWhitespace(String text) { + if (text == null) { + return null; + } return Arrays.stream(text.split("\n")) .map(String::trim) // Trim each line .collect(Collectors.joining("\n")); // Reconstruct text with normalized lines }tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/executable/cases/project/NewEspressifIDFProjectSDKconfigTest.java (2)
185-185: LGTM! Consider extracting the tab name and timeout as constants.The change is consistent with the previous one. To further improve maintainability, consider extracting the tab name "SDK Configuration (sdkconfig)" and timeout value 5000 as class constants since they're used multiple times in the test class.
public class NewEspressifIDFProjectSDKconfigTest { + private static final String SDK_CONFIG_TAB_NAME = "SDK Configuration (sdkconfig)"; + private static final int TAB_WAIT_TIMEOUT_MS = 5000; // ... existing code ... private static void whenSDKconfigFileOpenedUsingDoubleClick() throws IOException { bot.tree().getTreeItem(projectName).getNode("sdkconfig").doubleClick(); TestWidgetWaitUtility.waitWhileDialogIsVisible(bot, "Progress Information", 40000); - TestWidgetWaitUtility.waitForCTabToAppear(bot, "SDK Configuration (sdkconfig)", 5000); + TestWidgetWaitUtility.waitForCTabToAppear(bot, SDK_CONFIG_TAB_NAME, TAB_WAIT_TIMEOUT_MS); }
86-86: Consider replacing hardcoded sleep durations with explicit wait conditions.The test class uses hardcoded sleep durations in several places (e.g., lines 86, 217). These can make tests flaky or unnecessarily slow. Consider replacing them with explicit wait conditions using SWTBot's built-in condition checking mechanisms.
Example for line 86:
- bot.sleep(1000); + TestWidgetWaitUtility.waitForOperationsInProgressToFinishSync(bot);Also applies to: 217-217
tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/executable/cases/project/NewEspressifIDFProjectClangFilesTest.java (9)
131-148: Make file content validation more robust.The exact match comparison for file content is fragile and may break with whitespace or line ending differences.
Consider using normalized text comparison or pattern matching:
-assertTrue(ProjectTestOperations.checkExactMatchInTextEditor( +assertTrue(ProjectTestOperations.checkNormalizedTextInEditor(
114-189: Clean up or implement commented-out methods.Several methods are commented out (
thenClangdFileIsAbsent,whenClangdFileIsSaved, etc.). Either implement these methods or remove them to maintain code cleanliness.
190-196: Add cleanup verification.The cleanup method should verify that projects are actually deleted to ensure a clean state for subsequent tests.
private static void cleanTestEnv() { TestWidgetWaitUtility.waitForOperationsInProgressToFinishAsync(bot); ProjectTestOperations.closeAllProjects(bot); ProjectTestOperations.deleteAllProjects(bot); + assertTrue("Projects were not deleted", bot.tree().getAllItems().length == 0); }
77-77: Replace hardcoded sleep values with configurable constants.Using hardcoded sleep values (
bot.sleep(1000)) can make tests flaky. Consider extracting these values into named constants or using dynamic waits.+private static final int DEFAULT_WAIT_TIME = 1000; + private static void loadEnv() throws Exception { bot = WorkBenchSWTBot.getBot(); EnvSetupOperations.setupEspressifEnv(bot); - bot.sleep(1000); + bot.sleep(DEFAULT_WAIT_TIME); ProjectTestOperations.deleteAllProjects(bot); }Also applies to: 101-101
114-188: Remove or implement commented-out methods.The commented-out methods suggest incomplete features. Either implement these methods or remove them to maintain clean code.
106-106: Enhance test assertions for better robustness and maintainability.Consider these improvements:
- Use
assertNotNullwith descriptive messages for file existence checks- Use content comparison that's less sensitive to whitespace/formatting
-assertTrue(bot.tree().getTreeItem(projectName).getNode(".clangd") != null); +assertNotNull("Clangd file should exist", bot.tree().getTreeItem(projectName).getNode(".clangd")); -assertTrue(ProjectTestOperations.checkExactMatchInTextEditor( +assertTrue(ProjectTestOperations.checkNormalizedContentInTextEditor( "CompileFlags:\n" + " CompilationDatabase: build\n" + " Remove: [-m*, -f*]", bot));Also applies to: 111-111, 134-135, 141-147
114-117: Remove or complete commented-out code.Several methods are commented out without clear indication of their future use. Either complete these methods or remove them to maintain code cleanliness.
Also applies to: 150-154, 166-182, 184-188
77-77: Replace magic numbers with named constants.The sleep duration of 1000ms is hardcoded. Consider extracting it to a constant like
WAIT_AFTER_ACTION_MS.Also applies to: 101-101
131-136: Enhance file content verification.The current exact string matching is fragile. Consider:
- Using a more flexible comparison that ignores whitespace
- Validating the YAML structure for both files
- Moving the expected content to resource files
Example improvement:
+private static String normalizeContent(String content) { + return content.replaceAll("\\s+", " ").trim(); +} private static void thenClangdFileContentChecked() throws Exception { bot.cTabItem(".clangd").activate(); - assertTrue(ProjectTestOperations.checkExactMatchInTextEditor( - "CompileFlags:\n" + " CompilationDatabase: build\n" + " Remove: [-m*, -f*]", bot)); + String expectedContent = loadResourceFile("expected_clangd.yaml"); + String actualContent = bot.editor().getText(); + assertTrue("Content mismatch", + normalizeContent(actualContent).equals(normalizeContent(expectedContent))); }Also applies to: 138-148
tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/common/utility/TestWidgetWaitUtility.java (1)
273-289: Add method documentation.Add Javadoc to document the parameters and purpose of the
waitForCTabToAppearmethod, following the style used for other methods in this class.+/** + * Waits until the specified CTab item appears and is active + * + * @param workbenchBot current SWTWorkbenchBot reference + * @param tabName name of the tab to wait for + * @param timeout time to wait in ms before throwing {@link WidgetNotFoundException} + */ public static void waitForCTabToAppear(SWTWorkbenchBot workbenchBot, String tabName, long timeout)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/common/utility/TestWidgetWaitUtility.java(1 hunks)tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/executable/cases/project/NewEspressifIDFProjectClangFilesTest.java(1 hunks)tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/executable/cases/project/NewEspressifIDFProjectSDKconfigTest.java(1 hunks)tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/operations/ProjectTestOperations.java(2 hunks)
👮 Files not reviewed due to content moderation or server errors (2)
- tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/executable/cases/project/NewEspressifIDFProjectClangFilesTest.java
- tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/common/utility/TestWidgetWaitUtility.java
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: build
- GitHub Check: build_macos
🔇 Additional comments (7)
tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/operations/ProjectTestOperations.java (1)
257-271: LGTM! Well-structured text comparison implementation.The implementation correctly handles common text comparison challenges like line ending differences and whitespace variations. The method is well-documented with clear comments explaining the normalization steps.
tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/executable/cases/project/NewEspressifIDFProjectSDKconfigTest.java (1)
178-178: LGTM! Good refactoring to use a more generic method.The change improves maintainability by using a generic tab waiting utility instead of a hardcoded one.
tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/executable/cases/project/NewEspressifIDFProjectClangFilesTest.java (3)
1-48: LGTM! Well-structured test class setup.The test class follows JUnit best practices with proper setup/cleanup methods and enforced method ordering.
1-48: LGTM! Well-structured test class setup.The test class follows JUnit best practices with proper setup/teardown methods and enforced method ordering.
33-37: Well-structured test class!The class follows good testing practices with proper annotations and organization.
tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/common/utility/TestWidgetWaitUtility.java (2)
273-289: LGTM! Good refactoring for reusability.The method has been successfully generalized to work with any CTab, improving code reuse.
273-289: Good refactoring of the wait utility method!The changes make the method more generic and reusable while maintaining its core functionality. The error message is also properly updated to include the dynamic tab name.
| @Test | ||
| public void givenNewProjectIsCreatedThenTestFullSDKconfigWorkflowForNewProject() throws Exception | ||
| { | ||
| Fixture.givenNewEspressifIDFProjectIsSelected("EspressIf", "Espressif IDF Project"); | ||
| Fixture.givenProjectNameIs("NewProjectClangFilesTest"); | ||
| Fixture.whenNewProjectIsSelected(); | ||
| Fixture.thenClangdFileIsPresent(); | ||
| Fixture.whenClangdFileOpenedUsingDoubleClick(); | ||
| Fixture.thenClangdFileContentChecked(); | ||
| Fixture.thenClangdShellClosed(); | ||
| Fixture.thenClangFormatFileIsPresent(); | ||
| Fixture.whenClangFormatFileOpenedUsingDoubleClick(); | ||
| Fixture.thenClangFormatContentChecked(); | ||
| Fixture.thenClangFormatShellClosed(); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Rename test method to match its purpose.
The current method name givenNewProjectIsCreatedThenTestFullSDKconfigWorkflowForNewProject suggests it's testing SDKconfig workflow, but it's actually validating Clangd and Clang-Format files.
-public void givenNewProjectIsCreatedThenTestFullSDKconfigWorkflowForNewProject() throws Exception
+public void givenNewProjectIsCreatedThenValidateClangFilesExistWithCorrectContent() 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 givenNewProjectIsCreatedThenTestFullSDKconfigWorkflowForNewProject() throws Exception | |
| { | |
| Fixture.givenNewEspressifIDFProjectIsSelected("EspressIf", "Espressif IDF Project"); | |
| Fixture.givenProjectNameIs("NewProjectClangFilesTest"); | |
| Fixture.whenNewProjectIsSelected(); | |
| Fixture.thenClangdFileIsPresent(); | |
| Fixture.whenClangdFileOpenedUsingDoubleClick(); | |
| Fixture.thenClangdFileContentChecked(); | |
| Fixture.thenClangdShellClosed(); | |
| Fixture.thenClangFormatFileIsPresent(); | |
| Fixture.whenClangFormatFileOpenedUsingDoubleClick(); | |
| Fixture.thenClangFormatContentChecked(); | |
| Fixture.thenClangFormatShellClosed(); | |
| } | |
| @Test | |
| - public void givenNewProjectIsCreatedThenTestFullSDKconfigWorkflowForNewProject() throws Exception | |
| + public void givenNewProjectIsCreatedThenValidateClangFilesExistWithCorrectContent() throws Exception | |
| { | |
| Fixture.givenNewEspressifIDFProjectIsSelected("EspressIf", "Espressif IDF Project"); | |
| Fixture.givenProjectNameIs("NewProjectClangFilesTest"); | |
| Fixture.whenNewProjectIsSelected(); | |
| Fixture.thenClangdFileIsPresent(); | |
| Fixture.whenClangdFileOpenedUsingDoubleClick(); | |
| Fixture.thenClangdFileContentChecked(); | |
| Fixture.thenClangdShellClosed(); | |
| Fixture.thenClangFormatFileIsPresent(); | |
| Fixture.whenClangFormatFileOpenedUsingDoubleClick(); | |
| Fixture.thenClangFormatContentChecked(); | |
| Fixture.thenClangFormatShellClosed(); | |
| } |
🛠️ Refactor suggestion
Rename test method to match its functionality.
The current method name givenNewProjectIsCreatedThenTestFullSDKconfigWorkflowForNewProject suggests testing SDKconfig workflow, but the method actually tests Clangd and Clang-Format files.
-public void givenNewProjectIsCreatedThenTestFullSDKconfigWorkflowForNewProject() throws Exception
+public void givenNewProjectIsCreatedThenVerifyClangFilesExistenceAndContent() 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 givenNewProjectIsCreatedThenTestFullSDKconfigWorkflowForNewProject() throws Exception | |
| { | |
| Fixture.givenNewEspressifIDFProjectIsSelected("EspressIf", "Espressif IDF Project"); | |
| Fixture.givenProjectNameIs("NewProjectClangFilesTest"); | |
| Fixture.whenNewProjectIsSelected(); | |
| Fixture.thenClangdFileIsPresent(); | |
| Fixture.whenClangdFileOpenedUsingDoubleClick(); | |
| Fixture.thenClangdFileContentChecked(); | |
| Fixture.thenClangdShellClosed(); | |
| Fixture.thenClangFormatFileIsPresent(); | |
| Fixture.whenClangFormatFileOpenedUsingDoubleClick(); | |
| Fixture.thenClangFormatContentChecked(); | |
| Fixture.thenClangFormatShellClosed(); | |
| } | |
| @Test | |
| public void givenNewProjectIsCreatedThenVerifyClangFilesExistenceAndContent() throws Exception | |
| { | |
| Fixture.givenNewEspressifIDFProjectIsSelected("EspressIf", "Espressif IDF Project"); | |
| Fixture.givenProjectNameIs("NewProjectClangFilesTest"); | |
| Fixture.whenNewProjectIsSelected(); | |
| Fixture.thenClangdFileIsPresent(); | |
| Fixture.whenClangdFileOpenedUsingDoubleClick(); | |
| Fixture.thenClangdFileContentChecked(); | |
| Fixture.thenClangdShellClosed(); | |
| Fixture.thenClangFormatFileIsPresent(); | |
| Fixture.whenClangFormatFileOpenedUsingDoubleClick(); | |
| Fixture.thenClangFormatContentChecked(); | |
| Fixture.thenClangFormatShellClosed(); | |
| } |
🛠️ Refactor suggestion
Rename test method to match its purpose.
The method name givenNewProjectIsCreatedThenTestFullSDKconfigWorkflowForNewProject suggests it tests SDKconfig workflow, but it actually tests Clang files. Consider renaming to testClangFilesInNewProject or similar.
|
|
||
| private static void thenClangdFileIsPresent() throws IOException | ||
| { | ||
| assertTrue(bot.tree().getTreeItem(projectName).getNode(".clangd") != null); |
There was a problem hiding this comment.
Add null check for tree item access.
The getNode() call could throw NullPointerException if the tree item doesn't exist. Consider using a safer approach:
-assertTrue(bot.tree().getTreeItem(projectName).getNode(".clangd") != null);
+SWTBotTreeItem projectItem = bot.tree().getTreeItem(projectName);
+assertTrue("Project not found", projectItem != null);
+assertTrue("Clangd file not found", projectItem.getNode(".clangd") != null);Also applies to: 111-111
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/executable/cases/project/NewEspressifIDFProjectClangFilesTest.java (2)
50-64: 🛠️ Refactor suggestionRename test method to match its purpose.
The current method name
givenNewProjectIsCreatedThenTestClangFilesForNewProjectdoesn't clearly indicate that it's validating Clangd and Clang-Format files.-public void givenNewProjectIsCreatedThenTestClangFilesForNewProject() throws Exception +public void givenNewProjectIsCreatedThenValidateClangFilesExistWithCorrectContent() throws Exception
119-127:⚠️ Potential issueAdd null checks for tree item access.
The
getNode()calls could throw NullPointerException if the tree items don't exist.private static void thenClangdFileIsPresent() throws IOException { - assertTrue(bot.tree().getTreeItem(projectName).getNode(".clangd") != null); + SWTBotTreeItem projectItem = bot.tree().getTreeItem(projectName); + assertTrue("Project not found", projectItem != null); + assertTrue("Clangd file not found", projectItem.getNode(".clangd") != null); } private static void thenClangFormatFileIsPresent() throws IOException { - assertTrue(bot.tree().getTreeItem(projectName).getNode(".clang-format") != null); + SWTBotTreeItem projectItem = bot.tree().getTreeItem(projectName); + assertTrue("Project not found", projectItem != null); + assertTrue("Clang-format file not found", projectItem.getNode(".clang-format") != null); }
🧹 Nitpick comments (1)
tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/executable/cases/project/NewEspressifIDFProjectClangFilesTest.java (1)
180-219: Remove or implement commented-out methods.The commented-out methods suggest incomplete features for saving and verifying changes. Either implement these methods or remove them to maintain clean code.
📜 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(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- 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)
1-36: LGTM! Well-structured test class.The class is properly set up with necessary annotations and follows SWTBot test class best practices.
|
Hi @AndriiFilippov @alirana01 Can these test cases cover the following?
These are the real issues we have faced earlier by our users. Can you think of addressing them |
@kolipakakondal I think we also need to investigate if the clangd or llvm project itself handling these in their tests in terms of SWTBot I am not very confident with these. Until and unless the existing lsp editor is throwing back these errors there is no way to verify this for the positive case we can verify this with a constant formatted file in our tests against the formatted code in the tests.
In terms of complete end to end testing with SWTBot tests I believe we can only check for the progress of indexer and see if it is working and also verify that there are no errors reported for a project and navigation is working on the code. @AndriiFilippov
@AndriiFilippov @kolipakakondal I think the answer to first question should address this as well. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/operations/ProjectTestOperations.java (1)
168-202: Consider logging or handling the scenario whenmain.cis not found.If the file isn't found after several attempts, the loop exits quietly without logging or throwing an error, leading to uncertain test results. Logging or throwing an exception would clarify the outcome.
for (int attempt = 0; attempt <= maxAttempts; attempt++) { SWTBotTreeItem fileToOpenItem = findTreeItem(projectItem.getNode("main"), "main.c"); if (fileToOpenItem != null) { fileToOpenItem.select(); fileToOpenItem.doubleClick(); return; } else { Thread.sleep(3000); } } +logger.info("File 'main.c' was not found in the 'main' folder after {} attempts.", maxAttempts + 1);tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/executable/cases/project/NewEspressifIDFProjectClangFilesTest.java (1)
67-80: Test name is quite long.While descriptive, consider a shorter name that remains clear.
- givenNewProjectIsCreatedWhenClangdFileIsDeletedThenTestClangdFileCreatedUsingContextMenuThenClangdFileContentChecked + givenNewProjectWhenClangdDeletedThenClangdFileRecreatedAndValidated
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/executable/cases/project/NewEspressifIDFProjectClangFilesTest.java(1 hunks)tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/operations/ProjectTestOperations.java(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: build
- GitHub Check: build_macos
🔇 Additional comments (7)
tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/operations/ProjectTestOperations.java (2)
13-13: Import statement looks good.The newly introduced
Collectorsimport is properly used in thenormalizeWhitespacemethod.
293-326: Whitespace normalization approach is acceptable.The introduced methods for comparing text with or without whitespace differences are well-structured and serve the intended purpose. However, note that trimming each line may overlook trailing or intentional whitespace differences.
tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/executable/cases/project/NewEspressifIDFProjectClangFilesTest.java (5)
1-4: Validate copyright year.Confirm that “2025 Espressif Systems” is the intended copyright year, as future-dated licenses can occasionally be flagged.
39-65: Descriptive test name but verify coverage.The test name describes validating
.clangdand.clang-formatfiles. Ensure that if any additional clang-related files exist, they are covered as well.
82-97: Good coverage of.clang-formatsettings.This test ensures
.clang-formatis both edited and validated, which appears comprehensive for verifying real-world formatting scenarios.
99-176: Fixture class usage is appropriate.Centralizing setup and teardown logic in a
Fixtureclass is a valid design choice, especially for UI tests. Ensure that the environment cleanup does not overlap with other tests running in parallel.
177-202: Consider verifying the final check of.clangd/.clang-formatread actions.You're asserting content matches. If there's any asynchronous background indexing needed by the environment, consider adding a small wait or a more robust condition to ensure the files have settled before checking.
| + "PackConstructorInitializers: NextLineOnly\n" | ||
| + "BreakConstructorInitializers: AfterColon\n" + "IndentAccessModifiers: false\n" | ||
| + "AccessModifierOffset: -4"); | ||
| } | ||
|
|
||
| private static void addSpaceToMainFile() throws Exception | ||
| { | ||
| SWTBotEditor textEditor = bot.activeEditor(); | ||
| textEditor.toTextEditor() | ||
| .setText("#include <stdbool.h>\n" + "#include <stdio.h>\n" + "#include <unistd.h>\n\n" | ||
| + "void app_main(void) {\n" + "\t\twhile (true) {\n" | ||
| + "\t\t\tprintf(\"Hello from app_main!\\n\");\n" + // 2 tabs here | ||
| "\t\t\tsleep(1 );\n" + // 2 tabs here and extra spaces | ||
| "\t\t}\n" + // 2 tabs here | ||
| "}"); | ||
| } | ||
|
|
||
| private static void checkMainFileContentFormattedUnderActualSettings() throws Exception | ||
| { | ||
| bot.sleep(1000); | ||
| assertTrue(ProjectTestOperations.checkExactMatchInTextEditorwithWhiteSpaces("#include <stdbool.h>\n" | ||
| + "#include <stdio.h>\n" + "#include <unistd.h>\n\n" + "void app_main(void) {\n" | ||
| + "while (true) {\n" + "printf(\"Hello from app_main!\\n\");\n" + "sleep(1);\n" + "}\n" + "}", | ||
| bot)); | ||
| } | ||
|
|
||
| private static void setupAutoSave() 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(); | ||
| prefrencesShell.bot().checkBox("Format source code").click(); | ||
| prefrencesShell.bot().button("Apply and Close").click(); | ||
| } | ||
|
|
||
| private static void whenMainFileIsOpened() throws Exception | ||
| { | ||
| ProjectTestOperations.openMainFileInTextEditorUsingContextMenu(projectName, bot); | ||
| } | ||
|
|
||
| private static void thenClangdShellClosed() throws IOException | ||
| { | ||
| bot.cTabItem(".clangd").close(); | ||
| } | ||
|
|
||
| private static void thenClangFormatShellClosed() throws IOException | ||
| { | ||
| bot.cTabItem(".clang-format").close(); | ||
| } | ||
|
|
||
| private static void thenEditedClangFormatShellClosed() throws IOException | ||
| { | ||
| bot.cTabItem("*.clang-format").close(); | ||
| bot.shell("Save Resource").bot().button("Save").click(); | ||
| } | ||
|
|
||
| private static void thenMainFileShellClosed() throws IOException | ||
| { | ||
| bot.cTabItem("*main.c").close(); | ||
| bot.shell("Save Resource").bot().button("Save").click(); | ||
| } | ||
|
|
||
| private static void cleanTestEnv() | ||
| { | ||
| TestWidgetWaitUtility.waitForOperationsInProgressToFinishAsync(bot); | ||
| ProjectTestOperations.closeAllProjects(bot); | ||
| ProjectTestOperations.deleteAllProjects(bot); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Reliance on bot.sleep(...) calls.
UI tests often rely on explicit sleeps, but these may be fragile if the environment behaves unexpectedly slow or fast. Consider using stable wait conditions to detect readiness (e.g., waiting for a widget or job) rather than fixed sleeps.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/operations/ProjectTestOperations.java (1)
315-327: Consider standardizing method naming convention.The method implementation correctly preserves whitespace during comparison, which complements the previous method that trims whitespace. However, there's a minor inconsistency in the naming convention.
-public static boolean checkExactMatchInTextEditorwithWhiteSpaces(String phrase, SWTWorkbenchBot bot) +public static boolean checkExactMatchInTextEditorWithWhitespaces(String phrase, SWTWorkbenchBot bot)
📜 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/operations/ProjectTestOperations.java(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: build
- GitHub Check: build_macos
🔇 Additional comments (4)
tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/operations/ProjectTestOperations.java (4)
168-202: Good implementation of file opening functionality.This method is well-structured and follows the same pattern as the existing
openProjectComponentYMLFileInTextEditorUsingContextMenumethod. The retry mechanism is a good approach to handle potential timing issues in test environments.
293-307: Well-designed text comparison method with proper normalization.This method appropriately handles line ending normalization and whitespace trimming, which is essential for reliable text comparisons across different platforms (Windows, Linux, macOS). The comments clearly explain the purpose of each normalization step.
309-313: Effective use of Java streams for text processing.The implementation uses modern Java 8 features (streams and collectors) for efficient text processing, making the code more concise and readable.
329-339: Well-documented utility method for line ending normalization.The method includes proper JavaDoc comments explaining its purpose and function. The implementation correctly handles different line ending formats across platforms.
|
@kolipakakondal @alirana01 hi !
I have checked this repo llvm-project/tree/main/clang-tools-extra/clangd/test , different tests and could tell that clang-format itself does not have strict validation for the configuration file in terms of catching every possible invalid key in the file during runtime(at least I don't see it there) . If user provide invalid keys or malformed entries, clang-format typically ignores them without throwing a detailed error, but it might issue some warnings or warnings for unrecognized options.
Yes, test number 3 does it. I was not able to simulate pressing the shortcut "Ctrl+Shift+F" using the SWTBot framework or run the "contextMenu" method inside the editor. So, the only way to format the main.c file and check if the clang-format settings are applied is by activating "Window -> Preferences -> C/C++ -> Editor (LSP) -> Save Actions -> Format source code" which allows us to format the code every time changes are saved to the main.c file.
|
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)
162-165: Consider more consistent assertion methodsThis method uses a different approach to check for file absence compared to the presence checks. Consider using a more consistent approach.
-private static void thenClangdFileIsAbsent() throws IOException -{ - assertTrue(!bot.tree().getTreeItem(projectName).getNodes().contains(".clangd")); -} +private static void thenClangdFileIsAbsent() throws IOException +{ + SWTBotTreeItem projectItem = bot.tree().getTreeItem(projectName); + assertTrue("Project not found", projectItem != null); + assertTrue("Clangd file should not exist", !projectItem.getNodes().contains(".clangd")); +}
237-248: Consider extracting preference setup to a shared utility methodThis setup code might be useful in other test classes. Consider extracting it to a shared utility class for better reusability.
// Add this method to a utility class like ProjectTestOperations or create a new PreferencesSetupUtility class +public static void setupAutoFormatOnSave(SWTWorkbenchBot bot) 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(); + prefrencesShell.bot().checkBox("Format source code").click(); + prefrencesShell.bot().button("Apply and Close").click(); +} // Then replace the existing method with: +private static void setupAutoSave() throws Exception +{ + ProjectTestOperations.setupAutoFormatOnSave(bot); +}
📜 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(1 hunks)
🧰 Additional context used
🧬 Code Definitions (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/operations/ProjectTestOperations.java (1) (1)
ProjectTestOperations(51-741)tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/common/utility/TestWidgetWaitUtility.java (1) (1)
SuppressWarnings(32-306)tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/executable/cases/project/NewEspressifIDFProjectSDKconfigTest.java (2) (2)
SuppressWarnings(33-240)Fixture(75-239)
🔇 Additional comments (10)
tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/executable/cases/project/NewEspressifIDFProjectClangFilesTest.java (10)
58-60: Method name clearly describes test intentThe method name uses a clear Given-When-Then pattern which effectively communicates the test's purpose, addressing previous concerns about test method naming.
144-147: Add null check for tree item accessThe
getNode()call could throw NullPointerException if the tree item doesn't exist.-private static void thenClangdFileIsPresent() throws IOException -{ - assertTrue(bot.tree().getTreeItem(projectName).getNode(".clangd") != null); -} +private static void thenClangdFileIsPresent() throws IOException +{ + SWTBotTreeItem projectItem = bot.tree().getTreeItem(projectName); + assertTrue("Project not found", projectItem != null); + assertTrue("Clangd file not found", projectItem.getNode(".clangd") != null); +}
149-152: Add null check for tree item accessSimilar to the previous issue, this method also needs a null check to prevent potential NullPointerException.
-private static void thenClangFormatFileIsPresent() throws IOException -{ - assertTrue(bot.tree().getTreeItem(projectName).getNode(".clang-format") != null); -} +private static void thenClangFormatFileIsPresent() throws IOException +{ + SWTBotTreeItem projectItem = bot.tree().getTreeItem(projectName); + assertTrue("Project not found", projectItem != null); + assertTrue("Clang-format file not found", projectItem.getNode(".clang-format") != null); +}
186-191: Good implementation validating file contentThis method effectively addresses the concern raised in PR comments about validating the content of Clangd files, ensuring that they contain the expected configuration.
193-203: Good implementation validating Clang-Format contentThis method effectively addresses the concern raised in PR comments about validating the content of Clang-Format files, ensuring they contain the expected formatting rules.
216-226: Good test for Clang-Format impact on codeThis test effectively addresses the concern about validating how Clang-Format settings impact actual code formatting, by intentionally introducing spacing issues that should be fixed by the formatter.
228-235: Comprehensive end-to-end validationThis method verifies that the Clang-Format settings are correctly applied to the code, checking the actual formatted output against the expected result, which addresses the concern about validating the interaction between formatting tools and C/C++ code.
230-231: Replace explicit sleep with wait for a conditionInstead of using
bot.sleep(1000), consider using a more robust waiting mechanism like waiting for a specific condition to be met.-bot.sleep(1000); +// Use a more robust waiting mechanism like: +bot.waitUntil(new DefaultCondition() { + @Override + public boolean test() throws Exception { + return !bot.activeEditor().isDirty(); // Wait until editor is not dirty + } + + @Override + public String getFailureMessage() { + return "Editor did not finish formatting"; + } +}, 5000);
118-118: Consider using waitUntil instead of sleepUsing explicit sleep calls can make tests brittle. Consider using
bot.waitUntil()with appropriate conditions.-bot.sleep(1000); +bot.waitUntil(new DefaultCondition() { + @Override + public boolean test() throws Exception { + return ProjectTestOperations.areAllProjectsDeleted(bot); + } + + @Override + public String getFailureMessage() { + return "Projects were not deleted in time"; + } +}, 5000);
89-104: Well-designed test for validating Clang-Format behaviorThis test effectively validates that Clang-Format settings are properly applied to source code during editing, addressing a key concern raised in the PR comments.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/executable/cases/project/NewEspressifIDFProjectClangFilesTest.java (2)
145-147:⚠️ Potential issueAdd null check for tree item access.
The
getNode()call could throw NullPointerException if the tree item doesn't exist. Consider using a safer approach:- assertTrue(bot.tree().getTreeItem(projectName).getNode(".clangd") != null); + SWTBotTreeItem projectItem = bot.tree().getTreeItem(projectName); + assertTrue("Project not found", projectItem != null); + assertTrue("Clangd file not found", projectItem.getNode(".clangd") != null);
149-152:⚠️ Potential issueAdd null check for tree item access.
Similar to the previous comment, add a null check for the project tree item before accessing its node.
- assertTrue(bot.tree().getTreeItem(projectName).getNode(".clang-format") != null); + SWTBotTreeItem projectItem = bot.tree().getTreeItem(projectName); + assertTrue("Project not found", projectItem != null); + assertTrue("Clang-format file not found", projectItem.getNode(".clang-format") != null);
🧹 Nitpick comments (7)
tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/executable/cases/project/NewEspressifIDFProjectClangFilesTest.java (7)
1-4: Update copyright year to current year.The copyright year is set to 2025, which is incorrect as the current year is 2024.
-* Copyright 2025 Espressif Systems (Shanghai) PTE LTD. All rights reserved. +* Copyright 2024 Espressif Systems (Shanghai) PTE LTD. All rights reserved.
177-178: Extract timeout values to constants.Hardcoded timeout values reduce code maintainability. Extract these to class constants for better readability and easier adjustment.
+ private static final int DEFAULT_WAIT_TIMEOUT = 5000; + ... - TestWidgetWaitUtility.waitForCTabToAppear(bot, ".clangd", 5000); + TestWidgetWaitUtility.waitForCTabToAppear(bot, ".clangd", DEFAULT_WAIT_TIMEOUT);
194-205: Extract expected content to constants and remove redundant sleep.Long string literals in test code reduce readability and maintainability. Extract the expected content to constants and remove the redundant sleep call.
+ private static final String EXPECTED_CLANG_FORMAT_CONTENT = + "# We'll use defaults from the LLVM style, but with some modifications so that it's close to the CDT K&R style.\n" + + "BasedOnStyle: LLVM\n" + "UseTab: Always\n" + "IndentWidth: 4\n" + "TabWidth: 4\n" + + "PackConstructorInitializers: NextLineOnly\n" + + "BreakConstructorInitializers: AfterColon\n" + "IndentAccessModifiers: false\n" + + "AccessModifierOffset: -4"; ... private static void thenClangFormatContentChecked() throws Exception { bot.cTabItem(".clang-format").activate(); - assertTrue(ProjectTestOperations.checkExactMatchInTextEditor( - "# We'll use defaults from the LLVM style, but with some modifications so that it's close to the CDT K&R style.\n" - + "BasedOnStyle: LLVM\n" + "UseTab: Always\n" + "IndentWidth: 4\n" + "TabWidth: 4\n" - + "PackConstructorInitializers: NextLineOnly\n" - + "BreakConstructorInitializers: AfterColon\n" + "IndentAccessModifiers: false\n" - + "AccessModifierOffset: -4", - bot)); + assertTrue("Clang-format file content doesn't match expected content", + ProjectTestOperations.checkExactMatchInTextEditor(EXPECTED_CLANG_FORMAT_CONTENT, bot)); - bot.sleep(5000);
207-216: Extract modified content to constants for better maintainability.Similar to the previous comment, extract the modified content to a constant for better maintainability.
+ private static final String MODIFIED_CLANG_FORMAT_CONTENT = + "# We'll use defaults from the LLVM style, but with some modifications so that it's close to the CDT K&R style.\n" + + "BasedOnStyle: LLVM\n" + "UseTab: Always\n" + "IndentWidth: 0\n" + "TabWidth: 0\n" + + "PackConstructorInitializers: NextLineOnly\n" + + "BreakConstructorInitializers: AfterColon\n" + "IndentAccessModifiers: false\n" + + "AccessModifierOffset: -4"; ... private static void thenClangFormatContentEdited() throws Exception { SWTBotEditor textEditor = bot.activeEditor(); - textEditor.toTextEditor().setText( - "# We'll use defaults from the LLVM style, but with some modifications so that it's close to the CDT K&R style.\n" - + "BasedOnStyle: LLVM\n" + "UseTab: Always\n" + "IndentWidth: 0\n" + "TabWidth: 0\n" - + "PackConstructorInitializers: NextLineOnly\n" - + "BreakConstructorInitializers: AfterColon\n" + "IndentAccessModifiers: false\n" - + "AccessModifierOffset: -4"); + textEditor.toTextEditor().setText(MODIFIED_CLANG_FORMAT_CONTENT);
218-228: Extract test file content to constants.Extract the test file content to a constant for better maintainability.
+ private static final String MAIN_FILE_CONTENT_WITH_SPACES = + "#include <stdbool.h>\n" + "#include <stdio.h>\n" + "#include <unistd.h>\n\n" + + "void app_main(void) {\n" + "\t\twhile (true) {\n" + + "\t\t\tprintf(\"Hello from app_main!\\n\");\n" + + "\t\t\tsleep(1 );\n" + + "\t\t}\n" + + "}"; ... private static void addSpaceToMainFile() throws Exception { SWTBotEditor textEditor = bot.activeEditor(); - textEditor.toTextEditor() - .setText("#include <stdbool.h>\n" + "#include <stdio.h>\n" + "#include <unistd.h>\n\n" - + "void app_main(void) {\n" + "\t\twhile (true) {\n" - + "\t\t\tprintf(\"Hello from app_main!\\n\");\n" + // 2 tabs here - "\t\t\tsleep(1 );\n" + // 2 tabs here and extra spaces - "\t\t}\n" + // 2 tabs here - "}"); + textEditor.toTextEditor().setText(MAIN_FILE_CONTENT_WITH_SPACES);
230-237: Extract expected formatted content to constants and remove redundant sleep.Extract the expected formatted content to a constant and remove the redundant sleep call.
+ private static final String EXPECTED_FORMATTED_MAIN_FILE_CONTENT = + "#include <stdbool.h>\n" + "#include <stdio.h>\n" + "#include <unistd.h>\n\n" + + "void app_main(void) {\n" + "while (true) {\n" + "printf(\"Hello from app_main!\\n\");\n" + + "sleep(1);\n" + "}\n" + "}"; ... private static void checkMainFileContentFormattedUnderActualSettings() throws Exception { - bot.sleep(1000); - assertTrue(ProjectTestOperations.checkExactMatchInTextEditorwithWhiteSpaces("#include <stdbool.h>\n" - + "#include <stdio.h>\n" + "#include <unistd.h>\n\n" + "void app_main(void) {\n" - + "while (true) {\n" + "printf(\"Hello from app_main!\\n\");\n" + "sleep(1);\n" + "}\n" + "}", - bot)); + assertTrue("Main file content is not formatted as expected", + ProjectTestOperations.checkExactMatchInTextEditorwithWhiteSpaces( + EXPECTED_FORMATTED_MAIN_FILE_CONTENT, bot));
279-285: Add try-catch block in cleanTestEnv to ensure cleanup always happens.To make sure cleanup always happens regardless of any exceptions, add a try-catch block and log any errors.
private static void cleanTestEnv() { - TestWidgetWaitUtility.waitForOperationsInProgressToFinishAsync(bot); - ProjectTestOperations.closeAllProjects(bot); - ProjectTestOperations.deleteAllProjects(bot); + try { + TestWidgetWaitUtility.waitForOperationsInProgressToFinishAsync(bot); + ProjectTestOperations.closeAllProjects(bot); + ProjectTestOperations.deleteAllProjects(bot); + } catch (Exception e) { + System.err.println("Error during test environment cleanup: " + e.getMessage()); + }
📜 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(1 hunks)
🧰 Additional context used
🧬 Code Definitions (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/operations/ProjectTestOperations.java (1) (1)
ProjectTestOperations(51-741)tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/common/utility/TestWidgetWaitUtility.java (1) (1)
SuppressWarnings(32-306)tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/executable/cases/project/NewEspressifIDFProjectSDKconfigTest.java (2) (2)
SuppressWarnings(33-240)Fixture(75-239)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: build_macos
- GitHub Check: build
🔇 Additional comments (5)
tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/executable/cases/project/NewEspressifIDFProjectClangFilesTest.java (5)
58-72: The test method name effectively describes its purpose.This test method name clearly indicates what it's testing - the presence and content of Clang files in a new project. This is a significant improvement over previous versions where the method name suggested it was testing SDKconfig workflow.
117-118: Replace explicit sleep with wait condition.Using explicit sleep calls can make tests flaky if the environment behaves unexpectedly slow or fast. Consider using a wait condition that checks for a specific UI state instead.
- bot.sleep(1000); + TestWidgetWaitUtility.waitForOperationsInProgressToFinishSync(bot);
186-192: Remove redundant sleep call and improve assertion message.The sleep call after the assertion is redundant and could make the test slower than necessary. Also, add a descriptive message to the assertion for better error reporting.
bot.cTabItem(".clangd").activate(); - assertTrue(ProjectTestOperations.checkExactMatchInTextEditor( + assertTrue("Clangd file content doesn't match expected content", ProjectTestOperations.checkExactMatchInTextEditor( "CompileFlags:\n" + " CompilationDatabase: build\n" + " Remove: [-m*, -f*]", bot)); - bot.sleep(5000);
267-271: Add verification step for successful save.Currently, there's no verification that the save was successful. Consider adding a check to confirm the save operation completed without errors.
private static void thenEditedClangFormatShellClosed() throws IOException { bot.cTabItem("*.clang-format").close(); bot.shell("Save Resource").bot().button("Save").click(); + TestWidgetWaitUtility.waitForOperationsInProgressToFinishSync(bot); + // Verify save was successful - could check file modification time or reload and verify content
273-277: Add verification step for successful save.Similar to the previous comment, add verification that the save was successful.
private static void thenMainFileShellClosed() throws IOException { bot.cTabItem("*main.c").close(); bot.shell("Save Resource").bot().button("Save").click(); + TestWidgetWaitUtility.waitForOperationsInProgressToFinishSync(bot); + // Verify save was successful
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (7)
tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/executable/cases/project/NewEspressifIDFProjectClangFilesTest.java (7)
133-133: 🛠️ Refactor suggestionReplace explicit sleep with conditional wait.
Explicit sleeps can make tests flaky. Use a conditional wait instead of a fixed sleep duration.
- bot.sleep(1000); + TestWidgetWaitUtility.waitForOperationsInProgressToFinishSync(bot);
178-178: 🛠️ Refactor suggestionReplace explicit sleep with conditional wait.
Using explicit sleeps can make tests brittle. Consider using a wait condition or verify that the tree is fully expanded.
- bot.sleep(1000); + TestWidgetWaitUtility.waitForTreeItemExpansionToComplete(bot, projectName);Note: You may need to implement the
waitForTreeItemExpansionToCompletemethod in theTestWidgetWaitUtilityclass if it doesn't exist yet.
183-183:⚠️ Potential issueAdd null check for tree item access.
This operation can throw a NullPointerException if the node doesn't exist. Add proper null checks.
- assertTrue(bot.tree().getTreeItem(projectName).getNode(".clangd") != null); + org.eclipse.swtbot.swt.finder.widgets.SWTBotTreeItem projectItem = bot.tree().getTreeItem(projectName); + assertTrue("Project not found", projectItem != null); + assertTrue("Clangd file not found", projectItem.getNode(".clangd") != null);
188-188:⚠️ Potential issueAdd null check for tree item access.
This operation can throw a NullPointerException if the node doesn't exist. Add proper null checks.
- assertTrue(bot.tree().getTreeItem(projectName).getNode(".clang-format") != null); + org.eclipse.swtbot.swt.finder.widgets.SWTBotTreeItem projectItem = bot.tree().getTreeItem(projectName); + assertTrue("Project not found", projectItem != null); + assertTrue("Clang-format file not found", projectItem.getNode(".clang-format") != null);
196-196: 🛠️ Refactor suggestionReplace explicit sleep with dialog wait.
Use a wait for the deletion operation to complete instead of a fixed sleep.
- bot.sleep(1000); + TestWidgetWaitUtility.waitForOperationsInProgressToFinishSync(bot);
228-228: 🛠️ Refactor suggestionRemove unnecessary explicit sleeps.
Multiple lengthy sleeps (5000ms) make tests slower and more brittle. Either remove them or replace with specific wait conditions.
- bot.sleep(5000);Also applies to: 240-240, 267-267
352-356: 🛠️ Refactor suggestionReplace explicit sleep with conditional wait.
Use a wait for the dialog to close instead of a fixed sleep.
private static void closePreferencesDialog() { SWTBotShell preferencesShell = bot.shell("Preferences"); preferencesShell.bot().button("Cancel").click(); // Or "Apply and Close" - bot.sleep(1000); + TestWidgetWaitUtility.waitForDialogToClose(bot, "Preferences", 5000); }Note: You may need to implement the
waitForDialogToClosemethod in theTestWidgetWaitUtilityclass if it doesn't exist yet.
🧹 Nitpick comments (7)
tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/executable/cases/project/NewEspressifIDFProjectClangFilesTest.java (7)
2-2: Update copyright year.The copyright year is set to 2025, which is in the future. Update it to the current year (2024).
-* Copyright 2025 Espressif Systems (Shanghai) PTE LTD. All rights reserved. +* Copyright 2024 Espressif Systems (Shanghai) PTE LTD. All rights reserved.
84-85: Simplify long method name.This method name is excessively long which reduces readability. Maintain the BDD style but simplify it.
-public void givenNewProjectIsCreatedWhenClangdFileIsDeletedThenTestClangdFileCreatedUsingContextMenuThenClangdFileContentChecked() - throws Exception +public void testClangdFileRecreationAfterDeletion() throws Exception
201-201: Improve assertion message for clarity.Add a descriptive message to the assertion to aid in debugging if the test fails.
- assertTrue(!bot.tree().getTreeItem(projectName).getNodes().contains(".clangd")); + assertTrue("Clangd file should not be present after deletion", + !bot.tree().getTreeItem(projectName).getNodes().contains(".clangd"));
259-261: Use consistent indentation in test data.The indentation in the test string uses two tabs, which is inconsistent with your test expectation. Either document this or make it consistent.
Consider documenting the intentional inconsistency:
+ // Using intentionally inconsistent indentation to test formatting correction textEditor.toTextEditor() .setText("#include <stdbool.h>\n" + "#include <stdio.h>\n" + "#include <unistd.h>\n\n" + "void app_main(void) {\n" + "\t\twhile (true) {\n" + "\t\t\tprintf(\"Hello from app_main!\\n\");\n" + // 2 tabs here "\t\t\tsleep(1 );\n" + // 2 tabs here and extra spaces "\t\t}\n" + // 2 tabs here "}");
315-324: Improve method formatting and whitespace.The method has inconsistent indentation. Fix the formatting to match the rest of the codebase.
private static String getExpectedBuildFolderPATHforClangdAdditionalArgument(String projectName) throws IOException { - try { - IProject project = ResourcesPlugin.getWorkspace().getRoot().getProject(projectName); - String buildFolder = IDFUtil.getBuildDir(project); - Path buildFolderPath = Paths.get(buildFolder); - return "--compile-commands-dir=" + buildFolderPath.toAbsolutePath().toString(); - } catch (CoreException e) { - throw new IOException("Failed to get build directory for project: " + projectName, e); - } + try { + IProject project = ResourcesPlugin.getWorkspace().getRoot().getProject(projectName); + String buildFolder = IDFUtil.getBuildDir(project); + Path buildFolderPath = Paths.get(buildFolder); + return "--compile-commands-dir=" + buildFolderPath.toAbsolutePath().toString(); + } catch (CoreException e) { + throw new IOException("Failed to get build directory for project: " + projectName, e); + } }
326-332: Improve method formatting and whitespace.Fix the inconsistent whitespace and brace positioning in this method.
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)); + SWTBotShell prefrencesShell = bot.shell("Preferences"); + String actualClangdPath = prefrencesShell.bot().textWithLabel("Additional").getText(); + String expectedClangdPath = getExpectedBuildFolderPATHforClangdAdditionalArgument(projectName); + assertTrue("Expected and actual Clangd paths don't match", expectedClangdPath.equals(actualClangdPath)); }
1-365: Consider enhancing the test suite with additional validations.Based on the PR comments, consider adding these validations if not already covered elsewhere:
- Testing with invalid .clangd or .clang-format configurations to verify error handling
- More thorough validation of Clangd interaction with actual C/C++ code (e.g., error detection)
Would you like me to suggest specific test scenarios to address these validation cases?
📜 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(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: build
- GitHub Check: build_macos
🔇 Additional comments (2)
tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/executable/cases/project/NewEspressifIDFProjectClangFilesTest.java (2)
68-68: Method name correctly identifies purpose.The test method name is clear and follows the BDD convention. Good job updating it to match its functionality (testing Clang files rather than SDKconfig workflow).
68-120: Excellent test coverage for Clangd and Clang-Format functionality.The test suite comprehensively covers important scenarios:
- Verifying presence and content of default Clangd/Clang-Format files
- Testing recreation of deleted configuration files
- Validating that formatting settings are correctly applied to code
- Ensuring build folder paths are properly reflected in Clangd preferences
This addresses the reviewer questions about validating configuration content and formatting impact.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (5)
tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/executable/cases/project/NewEspressifIDFProjectClangFilesTest.java (5)
216-216:⚠️ Potential issueAdd null check for tree item access.
The
getNode()call could throw NullPointerException if the tree item doesn't exist.-assertTrue(bot.tree().getTreeItem(projectName).getNode(".clangd") != null); +SWTBotTreeItem projectItem = bot.tree().getTreeItem(projectName); +assertTrue("Project not found", projectItem != null); +assertTrue("Clangd file not found", projectItem.getNode(".clangd") != null);
221-221:⚠️ Potential issueAdd null check for tree item access.
The
getNode()call could throw NullPointerException if the tree item doesn't exist.-assertTrue(bot.tree().getTreeItem(projectName).getNode(".clang-format") != null); +SWTBotTreeItem projectItem = bot.tree().getTreeItem(projectName); +assertTrue("Project not found", projectItem != null); +assertTrue("Clang-format file not found", projectItem.getNode(".clang-format") != null);
198-198: 🛠️ Refactor suggestionReplace explicit sleep with wait conditions.
Using fixed sleep times can lead to flaky tests if the environment behaves unexpectedly slow or fast.
-bot.sleep(1000); +TestWidgetWaitUtility.waitForOperationsInProgressToFinishSync(bot);
152-152: 🛠️ Refactor suggestionReplace explicit sleep with wait conditions.
Using fixed sleep times can lead to flaky tests if the environment behaves unexpectedly slow or fast.
-bot.sleep(1000); +TestWidgetWaitUtility.waitForOperationsInProgressToFinishSync(bot);
303-303: 🛠️ Refactor suggestionReplace explicit sleep with content verification retry logic.
Using fixed sleep times can make tests unreliable. Consider implementing a retry mechanism that waits for the expected content to appear.
-bot.sleep(1000); +// Implement retry mechanism to check for content +int maxRetries = 10; +int retryInterval = 200; // milliseconds +boolean contentMatched = false; +for (int i = 0; i < maxRetries && !contentMatched; i++) { + contentMatched = ProjectTestOperations.checkExactMatchInTextEditorwithWhiteSpaces( + "#include <stdbool.h>\n" + "#include <stdio.h>\n" + "#include <unistd.h>\n\n" + + "void app_main(void) {\n" + "while (true) {\n" + + "printf(\"Hello from app_main!\\n\");\n" + "sleep(1);\n" + "}\n" + "}", bot); + if (!contentMatched && i < maxRetries - 1) { + TestWidgetWaitUtility.waitWhile(retryInterval); + } +} +assertTrue("Content did not match after multiple retries", contentMatched);
🧹 Nitpick comments (11)
tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/executable/cases/project/NewEspressifIDFProjectClangFilesTest.java (11)
71-71: Consider a more concise test method name.The method name is quite long. Consider a shorter but still descriptive name for better readability.
-public void givenNewProjectIsCreatedThenTestClangFilesPresenceAndContentForNewProject() throws Exception +public void testClangFilesExistenceAndContentInNewProject() throws Exception
87-88: Improve method name readability with proper line breaks.The method name is extremely long and spans two lines, making it hard to read.
-public void givenNewProjectIsCreatedWhenClangdFileIsDeletedThenTestClangdFileCreatedUsingContextMenuThenClangdFileContentChecked() - throws Exception +public void testClangdFileRecreationUsingContextMenu() throws Exception
130-130: Simplify long method name for better readability.This method name contains too many "Then" and "When" clauses, making it difficult to understand.
-public void givenNewProjectIsCreatedWhenBuiltThenClangdDriversUpdateOnTargetWhenChangeTargetAndBuiltThenClangdDriversUpdateOnTargetChange() throws Exception +public void testClangdDriversUpdateOnTargetChange() throws Exception
306-307: Extract test data to constant for better maintainability.Long string literals embedded in the code make it hard to maintain if the expected format changes.
+private static final String EXPECTED_FORMATTED_MAIN_FILE_CONTENT = + "#include <stdbool.h>\n" + + "#include <stdio.h>\n" + + "#include <unistd.h>\n\n" + + "void app_main(void) {\n" + + "while (true) {\n" + + "printf(\"Hello from app_main!\\n\");\n" + + "sleep(1);\n" + + "}\n" + + "}"; // Then in the method: -assertTrue(ProjectTestOperations.checkExactMatchInTextEditorwithWhiteSpaces("#include <stdbool.h>\n" - + "#include <stdio.h>\n" + "#include <unistd.h>\n\n" + "void app_main(void) {\n" - + "while (true) {\n" + "printf(\"Hello from app_main!\\n\");\n" + "sleep(1);\n" + "}\n" + "}", - bot)); +assertTrue(ProjectTestOperations.checkExactMatchInTextEditorwithWhiteSpaces(EXPECTED_FORMATTED_MAIN_FILE_CONTENT, bot));
350-360: Improve method naming and formatting for consistency.The method name has inconsistent capitalization and the formatting is inconsistent with the rest of the codebase.
-private static String getExpectedBuildFolderPATHforClangdAdditionalArgument(String projectName) throws IOException +private static String getExpectedBuildFolderPathForClangdAdditionalArgument(String projectName) throws IOException { - try { - IProject project = ResourcesPlugin.getWorkspace().getRoot().getProject(projectName); - String buildFolder = IDFUtil.getBuildDir(project); - Path buildFolderPath = Paths.get(buildFolder); - return "--compile-commands-dir=" + buildFolderPath.toAbsolutePath().toString(); - } catch (CoreException e) { - throw new IOException("Failed to get build directory for project: " + projectName, e); - } + try { + IProject project = ResourcesPlugin.getWorkspace().getRoot().getProject(projectName); + String buildFolder = IDFUtil.getBuildDir(project); + Path buildFolderPath = Paths.get(buildFolder); + return "--compile-commands-dir=" + buildFolderPath.toAbsolutePath().toString(); + } catch (CoreException e) { + throw new IOException("Failed to get build directory for project: " + projectName, e); + } }
362-367: Improve method formatting for consistency.The method formatting is inconsistent with the rest of the codebase.
private static String getXtensaToolchainPathBasedOnTheTargetConfigured() throws IOException { - String toolchain = IDFUtil.getToolchainExePathForActiveTarget(); - Path toolchainPath = Paths.get(toolchain); - return toolchainPath.toAbsolutePath().toString(); + String toolchain = IDFUtil.getToolchainExePathForActiveTarget(); + Path toolchainPath = Paths.get(toolchain); + return toolchainPath.toAbsolutePath().toString(); }
369-375: Fix formatting inconsistencies and improve assertion message.The method has inconsistent indentation and lacks a descriptive assertion message.
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)); + SWTBotShell prefrencesShell = bot.shell("Preferences"); + String actualClangdPath = prefrencesShell.bot().textWithLabel("Additional").getText(); + String expectedClangdPath = getExpectedBuildFolderPathForClangdAdditionalArgument(projectName); + assertTrue("Expected clangd argument does not match actual argument", expectedClangdPath.equals(actualClangdPath)); }
377-383: Fix formatting inconsistencies and improve assertion message.The method has inconsistent indentation and lacks a descriptive assertion message.
private static void thenCompareActualClangdDriversWithExpected(String projectName) throws IOException { -SWTBotShell prefrencesShell = bot.shell("Preferences"); -String actualClangdPath = prefrencesShell.bot().textWithLabel("Drivers").getText(); -String expectedClangdPath = getXtensaToolchainPathBasedOnTheTargetConfigured(); -assertTrue(expectedClangdPath.equals(actualClangdPath)); + SWTBotShell prefrencesShell = bot.shell("Preferences"); + String actualClangdPath = prefrencesShell.bot().textWithLabel("Drivers").getText(); + String expectedClangdPath = getXtensaToolchainPathBasedOnTheTargetConfigured(); + assertTrue("Expected clangd drivers path does not match actual path", expectedClangdPath.equals(actualClangdPath)); }
403-407: Clarify intent of the button click in comment.The comment
// Or "Apply and Close"might cause confusion about which button to click.private static void closePreferencesDialog() { SWTBotShell preferencesShell = bot.shell("Preferences"); - preferencesShell.bot().button("Cancel").click(); // Or "Apply and Close" + // Using Cancel to close without saving changes + preferencesShell.bot().button("Cancel").click(); TestWidgetWaitUtility.waitWhileDialogIsVisible(bot, "Preferences", 10000); }
409-414: Clarify intent of the button click in comment.The comment
// Or "Apply and Close"might cause confusion about which button to click.private static void closeTargetDialog() { TestWidgetWaitUtility.waitForDialogToAppear(bot, "IDF Launch Target Changed", 10000); SWTBotShell preferencesShell = bot.shell("IDF Launch Target Changed"); - preferencesShell.bot().button("Yes").click(); // Or "Apply and Close" + // Clicking "Yes" to confirm target change + preferencesShell.bot().button("Yes").click(); TestWidgetWaitUtility.waitWhileDialogIsVisible(bot, "IDF Launch Target Changed", 10000); }
1-423: Tests should verify the content and behavior of .clangd and .clang-format files.Based on the PR discussion, there were questions about validating the content of .clangd and .clang-format files, and their interaction with C/C++ code. The tests appear to cover these aspects, but it would be beneficial to add more specific tests for:
- Testing with invalid .clangd/.clang-format files to verify error handling
- More extensive validation of the impact of .clang-format settings on different code formatting scenarios
- Testing interactions between clangd and C/C++ code, including error detection and indexing
Would you like me to suggest specific test cases for these scenarios?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/executable/cases/project/NewEspressifIDFProjectClangFilesTest.java(1 hunks)tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/operations/ProjectTestOperations.java(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/operations/ProjectTestOperations.java
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: build_macos
- GitHub Check: build
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (6)
tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/executable/cases/project/NewEspressifIDFProjectClangFilesTest.java (6)
71-71: Rename test method to match its purpose.The current method name
givenNewProjectIsCreatedThenTestClangFilesPresenceAndContentForNewProjectsuggests it's testing SDKconfig workflow, but it's actually validating Clangd and Clang-Format files.-public void givenNewProjectIsCreatedThenTestClangFilesPresenceAndContentForNewProject() throws Exception +public void givenNewProjectIsCreatedThenVerifyClangFilesExistenceAndContent() throws Exception
152-152: Replace fixed sleep with wait condition.Using fixed sleep times can make tests fragile. Consider using SWTBot's wait conditions instead.
-bot.sleep(1000); +TestWidgetWaitUtility.waitForOperationsInProgressToFinishSync(bot);
198-198: Replace fixed sleep with wait condition.Similar to above, using a fixed sleep time can make tests fragile. Consider using wait conditions.
-bot.sleep(1000); +TestWidgetWaitUtility.waitForTreeItemToExpand(bot, projectName);
303-303: Replace fixed sleep with wait condition.Using fixed sleep times can make tests fragile. Consider using a wait condition that checks for editor readiness.
-bot.sleep(1000); +TestWidgetWaitUtility.waitForEditorContentToStabilize(bot);
216-216: 🛠️ Refactor suggestionAdd null check for tree item access.
The
getNode()call could throw NullPointerException if the tree item doesn't exist. Consider using a safer approach:-assertTrue(bot.tree().getTreeItem(projectName).getNode(".clangd") != null); +SWTBotTreeItem projectItem = bot.tree().getTreeItem(projectName); +assertTrue("Project not found", projectItem != null); +assertTrue("Clangd file not found", projectItem.getNode(".clangd") != null);
221-221: 🛠️ Refactor suggestionAdd null check for tree item access.
Similar to the previous comment, add proper null checks to prevent NullPointerException.
-assertTrue(bot.tree().getTreeItem(projectName).getNode(".clang-format") != null); +SWTBotTreeItem projectItem = bot.tree().getTreeItem(projectName); +assertTrue("Project not found", projectItem != null); +assertTrue("Clang-format file not found", projectItem.getNode(".clang-format") != null);
🧹 Nitpick comments (7)
tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/executable/cases/project/NewEspressifIDFProjectClangFilesTest.java (7)
102-102: Consider renaming method for better clarity.The method name
givenNewProjectIsCreatedThenTestClangFormatFileSettingsAreBeingAppliedis verbose. A more concise name would improve readability.-public void givenNewProjectIsCreatedThenTestClangFormatFileSettingsAreBeingApplied() throws Exception +public void testClangFormatSettingsAreAppliedToSourceFiles() throws Exception
129-131: Improve method name readability.The current method name is extremely long and difficult to read. Consider breaking it down or simplifying it.
-public void givenNewProjectIsCreatedWhenBuiltThenClangdDriversUpdateOnTargetWhenChangeTargetAndBuiltThenClangdDriversUpdateOnTargetChange() throws Exception +public void testClangdDriversUpdateWhenTargetChanges() throws Exception
234-234: Improve assertion message for better test diagnostics.The current assertion lacks a descriptive message that would help identify the failure.
-assertTrue(!bot.tree().getTreeItem(projectName).getNodes().contains(".clangd")); +assertTrue("Clangd file should be absent but was found", + !bot.tree().getTreeItem(projectName).getNodes().contains(".clangd"));
352-359: Improve exception handling and formatting.The try-catch block has inconsistent indentation and could use a more specific exception type.
- try { - IProject project = ResourcesPlugin.getWorkspace().getRoot().getProject(projectName); - String buildFolder = IDFUtil.getBuildDir(project); - Path buildFolderPath = Paths.get(buildFolder); - return "--compile-commands-dir=" + buildFolderPath.toAbsolutePath().toString(); - } catch (CoreException e) { - throw new IOException("Failed to get build directory for project: " + projectName, e); - } + try { + IProject project = ResourcesPlugin.getWorkspace().getRoot().getProject(projectName); + String buildFolder = IDFUtil.getBuildDir(project); + Path buildFolderPath = Paths.get(buildFolder); + return "--compile-commands-dir=" + buildFolderPath.toAbsolutePath().toString(); + } catch (CoreException e) { + throw new IOException("Failed to get build directory for project: " + projectName, e); + }
369-375: Add descriptive error message to assertion.The assertion doesn't include a descriptive message, which makes test failures harder to diagnose.
-assertTrue(expectedClangdPath.equals(actualClangdPath)); +assertTrue("Expected Clangd path '" + expectedClangdPath + + "' doesn't match actual path '" + actualClangdPath + "'", + expectedClangdPath.equals(actualClangdPath));
377-383: Add descriptive error message to assertion.Similar to the previous comment, add a descriptive message to the assertion.
-assertTrue(expectedClangdPath.equals(actualClangdPath)); +assertTrue("Expected Clangd driver path '" + expectedClangdPath + + "' doesn't match actual path '" + actualClangdPath + "'", + expectedClangdPath.equals(actualClangdPath));
403-407: Consider parameterizing the close method.The
closePreferencesDialogmethod could be parameterized to handle different dialog scenarios.-private static void closePreferencesDialog() { - SWTBotShell preferencesShell = bot.shell("Preferences"); - preferencesShell.bot().button("Cancel").click(); // Or "Apply and Close" - TestWidgetWaitUtility.waitWhileDialogIsVisible(bot, "Preferences", 10000); +private static void closeDialog(String dialogTitle, String buttonLabel) { + SWTBotShell shell = bot.shell(dialogTitle); + shell.bot().button(buttonLabel).click(); + TestWidgetWaitUtility.waitWhileDialogIsVisible(bot, dialogTitle, 10000);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/common/utility/TestWidgetWaitUtility.java(1 hunks)tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/executable/cases/project/NewEspressifIDFProjectClangFilesTest.java(1 hunks)tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/executable/cases/project/NewEspressifIDFProjectSDKconfigTest.java(4 hunks)tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/operations/ProjectTestOperations.java(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/common/utility/TestWidgetWaitUtility.java
- tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/executable/cases/project/NewEspressifIDFProjectSDKconfigTest.java
- tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/operations/ProjectTestOperations.java
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: build_macos
- GitHub Check: build
🔇 Additional comments (3)
tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/executable/cases/project/NewEspressifIDFProjectClangFilesTest.java (3)
72-83: LGTM - Good test structure.The test follows a clear Given-When-Then pattern and properly validates both the presence and content of the required config files.
90-98: LGTM - Good test case for deletion and recreation.This test properly validates the scenario where a config file is deleted and recreated using the context menu.
258-261: LGTM - Good content validation approach.The use of
checkExactMatchInTextEditorfor validating file content is a robust approach.
AndriiFilippov
force-pushed
the
IEP-1428
branch
2 times, most recently
from
90ac4bb to
2fe2261
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/executable/cases/project/NewEspressifIDFProjectClangFilesTest.java (3)
70-72: Rename test method to match its purpose.The method name
givenNewProjectIsCreatedThenTestClangFilesPresenceAndContentForNewProjectsuggests testing SDKconfig workflow, but it actually tests Clang files.-public void givenNewProjectIsCreatedThenTestClangFilesPresenceAndContentForNewProject() throws Exception +public void givenNewProjectIsCreatedThenVerifyClangFilesExistenceAndContent() throws Exception
215-216: Add null check for tree item access.The
getNode()call could throw NullPointerException if the tree item doesn't exist.-assertTrue(bot.tree().getTreeItem(projectName).getNode(".clangd") != null); +SWTBotTreeItem projectItem = bot.tree().getTreeItem(projectName); +assertTrue("Project not found", projectItem != null); +assertTrue("Clangd file not found", projectItem.getNode(".clangd") != null);
152-153: Reliance onbot.sleep(...)calls.UI tests often rely on explicit sleeps, but these may be fragile if the environment behaves unexpectedly slow or fast.
Consider using stable wait conditions to detect readiness (e.g., waiting for a widget or job) rather than fixed sleeps. For example, you're already using
TestWidgetWaitUtilitymethods elsewhere in the code.Also applies to: 198-199, 303-304
🧹 Nitpick comments (3)
tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/executable/cases/project/NewEspressifIDFProjectClangFilesTest.java (3)
1-4: Fix the copyright year.The copyright year is set to 2025, which is in the future. Update it to the current year (2023).
-* Copyright 2025 Espressif Systems (Shanghai) PTE LTD. All rights reserved. +* Copyright 2023 Espressif Systems (Shanghai) PTE LTD. All rights reserved.
102-116: Consider expanding format test coverage.The current test only verifies that indentation settings are applied correctly. Consider expanding the test to validate additional formatting properties from .clang-format (e.g., brace placement, space before parentheses).
Adding assertions for multiple formatting aspects would provide more comprehensive validation of the formatting functionality, addressing one of the key questions raised in PR comments about "validation of the impact of .clang-format settings on code formatting."
302-308: Add a comment explaining the specific formatting being tested.The test validates that the formatting settings (IndentWidth: 0, TabWidth: 0) are correctly applied, but it's not immediately clear why the expected result looks the way it does.
Add a comment explaining which specific formatting rules are being tested and how the expected result demonstrates that these rules are correctly applied. This would make the test more maintainable and educational for other developers.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
📒 Files selected for processing (1)
tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/executable/cases/project/NewEspressifIDFProjectClangFilesTest.java(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: build
- 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)
118-138: Clarify status of commented-out tests.There are two test methods that are commented out. If these tests are intended for future implementation, consider adding a TODO comment explaining why they're commented out and when they should be enabled.
Are these tests intended to be included in this PR or are they planned for future implementation? If they're not ready yet, consider adding TODO comments to clarify their status.
| private static void thenClangFormatFileIsPresent() throws IOException | ||
| { | ||
| assertTrue(bot.tree().getTreeItem(projectName).getNode(".clang-format") != null); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add null check for tree item access in format file verification.
Similar to the clangd file check, the getNode() call for .clang-format could throw NullPointerException.
-assertTrue(bot.tree().getTreeItem(projectName).getNode(".clang-format") != null);
+SWTBotTreeItem projectItem = bot.tree().getTreeItem(projectName);
+assertTrue("Project not found", projectItem != null);
+assertTrue("Clang-format file not found", projectItem.getNode(".clang-format") != null);📝 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 thenClangFormatFileIsPresent() throws IOException | |
| { | |
| assertTrue(bot.tree().getTreeItem(projectName).getNode(".clang-format") != null); | |
| } | |
| private static void thenClangFormatFileIsPresent() throws IOException | |
| { | |
| SWTBotTreeItem projectItem = bot.tree().getTreeItem(projectName); | |
| assertTrue("Project not found", projectItem != null); | |
| assertTrue("Clang-format file not found", projectItem.getNode(".clang-format") != null); | |
| } |
🤖 Prompt for AI Agents
In
tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/executable/cases/project/NewEspressifIDFProjectClangFilesTest.java
around lines 219 to 222, the method thenClangFormatFileIsPresent() directly
calls getNode(".clang-format") on the result of getTreeItem(projectName) without
checking if getTreeItem(projectName) is null. To fix this, add a null check for
the tree item before calling getNode() to prevent a NullPointerException,
similar to the approach used in the clangd file check.
…t targets. Commented lines in WaitForProjectBuild method
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (5)
tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/executable/cases/project/NewEspressifIDFProjectClangFilesTest.java (5)
216-216: Add null check for tree item accessThe
getNode()call could throw NullPointerException if the tree item doesn't exist. Consider using a safer approach.-assertTrue(bot.tree().getTreeItem(projectName).getNode(".clangd") != null); +SWTBotTreeItem projectItem = bot.tree().getTreeItem(projectName); +assertTrue("Project not found", projectItem != null); +assertTrue("Clangd file not found", projectItem.getNode(".clangd") != null);
221-221: Add null check for tree item access in format file verificationSimilar to the clangd file check, the getNode() call for .clang-format could throw NullPointerException.
-assertTrue(bot.tree().getTreeItem(projectName).getNode(".clang-format") != null); +SWTBotTreeItem projectItem = bot.tree().getTreeItem(projectName); +assertTrue("Project not found", projectItem != null); +assertTrue("Clang-format file not found", projectItem.getNode(".clang-format") != null);
303-303: 🛠️ Refactor suggestionReplace hard-coded sleep with wait condition
Use an explicit wait condition instead of a fixed sleep time.
-bot.sleep(1000); +TestWidgetWaitUtility.waitForEditorToLoad(bot);You may need to implement the
waitForEditorToLoadmethod if it doesn't exist yet.
152-152: 🛠️ Refactor suggestionAvoid hard-coded sleep times
Using fixed sleep times can make tests brittle. The test may fail if the operation takes longer than expected, or waste time if it completes quickly.
-bot.sleep(1000); +TestWidgetWaitUtility.waitForOperationsInProgressToFinishSync(bot);
198-198: 🛠️ Refactor suggestionReplace hard-coded sleep with explicit wait condition
Similar to the previous comment, this sleep should be replaced with an explicit wait condition.
-bot.sleep(1000); +TestWidgetWaitUtility.waitForTreeItemToExpand(bot, projectName);You may need to implement the
waitForTreeItemToExpandmethod if it doesn't exist yet.
🧹 Nitpick comments (8)
tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/executable/cases/project/NewEspressifIDFProjectClangFilesTest.java (8)
70-72: Improved test method namingThe method name has been improved compared to earlier versions but is still quite long. Consider a more concise naming pattern while preserving the BDD style.
-public void givenNewProjectIsCreatedThenTestClangFilesPresenceAndContentForNewProject() throws Exception +public void testClangFilesPresenceAndContent() throws Exception
87-89: Excessively long method nameThis method name is extremely long and could be difficult to read in test reports or IDE navigation.
-public void givenNewProjectIsCreatedWhenClangdFileIsDeletedThenTestClangdFileCreatedUsingContextMenuThenClangdFileContentChecked() +public void testClangdFileRecreationAfterDeletion()
369-375: Add meaningful assertion message and fix indentationThe assertion lacks a descriptive message, and the method has inconsistent indentation.
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)); + SWTBotShell prefrencesShell = bot.shell("Preferences"); + String actualClangdPath = prefrencesShell.bot().textWithLabel("Additional").getText(); + String expectedClangdPath = getExpectedBuildFolderPATHforClangdAdditionalArgument(projectName); + assertTrue("Expected clangd path '" + expectedClangdPath + "' but was '" + actualClangdPath + "'", + expectedClangdPath.equals(actualClangdPath)); }
377-383: Add meaningful assertion message and fix indentationSimilar to the previous method, add a descriptive assertion message and fix indentation.
private static void thenCompareActualClangdDriversWithExpected(String projectName) throws IOException { -SWTBotShell prefrencesShell = bot.shell("Preferences"); -String actualClangdPath = prefrencesShell.bot().textWithLabel("Drivers").getText(); -String expectedClangdPath = getXtensaToolchainPathBasedOnTheTargetConfigured(); -assertTrue(expectedClangdPath.equals(actualClangdPath)); + SWTBotShell prefrencesShell = bot.shell("Preferences"); + String actualClangdPath = prefrencesShell.bot().textWithLabel("Drivers").getText(); + String expectedClangdPath = getXtensaToolchainPathBasedOnTheTargetConfigured(); + assertTrue("Expected clangd drivers path '" + expectedClangdPath + "' but was '" + actualClangdPath + "'", + expectedClangdPath.equals(actualClangdPath)); }
405-405: Clarify button selection in commentThe comment suggests uncertainty about which button to click. Consider removing the comment or making a deliberate choice.
- preferencesShell.bot().button("Cancel").click(); // Or "Apply and Close" + preferencesShell.bot().button("Cancel").click(); // Using Cancel to avoid saving changes
412-412: Clarify button selection in commentSimilar to the previous comment, remove or clarify the ambiguous comment.
- preferencesShell.bot().button("Yes").click(); // Or "Apply and Close" + preferencesShell.bot().button("Yes").click(); // Confirm target change
355-356: Use Path.resolve instead of string concatenationInstead of string concatenation for path construction, use
Path.resolve()for better platform independence.-return "--compile-commands-dir=" + buildFolderPath.toAbsolutePath().toString(); +return "--compile-commands-dir=" + buildFolderPath.toAbsolutePath().normalize().toString();
129-131: Excessively long method nameThis method name is extremely long and difficult to read. Consider simplifying while preserving the test intent.
-public void givenNewProjectIsCreatedWhenBuiltThenClangdDriversUpdateOnTargetWhenChangeTargetAndBuiltThenClangdDriversUpdateOnTargetChange() throws Exception +public void testClangdDriversUpdateWhenTargetChanges() throws Exception
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
📒 Files selected for processing (4)
tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/common/utility/TestWidgetWaitUtility.java(1 hunks)tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/executable/cases/project/NewEspressifIDFProjectClangFilesTest.java(1 hunks)tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/executable/cases/project/NewEspressifIDFProjectSDKconfigTest.java(4 hunks)tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/operations/ProjectTestOperations.java(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/common/utility/TestWidgetWaitUtility.java
- tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/executable/cases/project/NewEspressifIDFProjectSDKconfigTest.java
- tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/operations/ProjectTestOperations.java
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: build_macos
- GitHub Check: build
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (4)
tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/executable/cases/project/NewEspressifIDFProjectClangFilesTest.java (4)
214-218: Null-safety for tree item access (repeat finding)
bot.tree().getTreeItem(projectName).getNode(".clangd")may throwWidgetNotFoundException(NPE in headless build) when the project or node is missing.
Previous review already highlighted this; please add null checks or use the safer utility:SWTBotTreeItem projectItem = ProjectTestOperations.fetchProjectFromProjectExplorer(projectName, bot); assertTrue("Project not found", projectItem != null); assertTrue("Clangd file not found", projectItem.getNode(".clangd") != null);
219-222: Same null-safety issue for .clang-format nodeApplies verbatim to
.clang-format; use the same guarded pattern as above.
196-199: 🛠️ Refactor suggestionExplicit wait preferable to fixed delay
Same rationale as above: replace
bot.sleep(1000)with a widget/operation wait that guarantees the project tree is populated before the next action.
Consider waiting for the.clangdtree node to appear:- bot.sleep(1000); + TestWidgetWaitUtility.waitForTreeItem(".clangd", bot.tree(), bot);
148-154: 🛠️ Refactor suggestionAvoid brittle fixed sleeps – switch to deterministic waits
bot.sleep(1000);introduces flakiness: the UI might need more/less time on slower/faster machines.
Use an existing helper such asTestWidgetWaitUtility.waitForOperationsInProgressToFinishSync(bot)or a purpose-built wait that asserts the workspace is idle before continuing.- bot.sleep(1000); + TestWidgetWaitUtility.waitForOperationsInProgressToFinishSync(bot);
🧹 Nitpick comments (3)
tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/executable/cases/project/NewEspressifIDFProjectClangFilesTest.java (3)
299-307: Remember to save the edited.clang-formatcontent
setTextchanges the editor buffer, but it is not persisted until saved.
AlthoughthenEditedClangFormatShellClosed()later triggers a save dialog, callingtextEditor.save()here would make the intent explicit and avoid data-loss if the dialog changes.textEditor.toTextEditor().setText(...); +textEditor.save();
331-340: Checkbox click may unintentionally toggle an already-enabled option
prefrencesShell.bot().checkBox("Format source code").click();will disable formatting if it was already enabled.
Guard the click with a state check:-if (!prefrencesShell.bot().checkBox("Format source code").isChecked()) -{ - prefrencesShell.bot().checkBox("Format source code").click(); -} +SWTBotCheckBox formatCb = prefrencesShell.bot().checkBox("Format source code"); +if (!formatCb.isChecked()) { + formatCb.click(); +}
423-427: Use “Apply and Close” when verifying preference-dependent assertionsThe comparison methods read values directly from the dialog, but callers elsewhere might expect persisted changes.
Consider using “Apply and Close” to guarantee any modifications are written, unless cancellation is intentional.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
📒 Files selected for processing (1)
tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/executable/cases/project/NewEspressifIDFProjectClangFilesTest.java(1 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)
bundles/com.espressif.idf.core/src/com/espressif/idf/core/util/IDFUtil.java (1)
IDFUtil(59-882)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(12-85)tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/operations/ProjectTestOperations.java (1)
ProjectTestOperations(51-742)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: build_macos
- GitHub Check: build
🔇 Additional comments (1)
tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/executable/cases/project/NewEspressifIDFProjectClangFilesTest.java (1)
430-434: Guard against missing “IDF Launch Target Changed” dialogIf the target switch happens silently (e.g., already selected), waiting for the dialog can hang.
Add a timeout fallback or checkbot.shells().stream().anyMatch(s -> s.getText().equals(...))before waiting.
| bot.sleep(1000); | ||
| assertTrue(ProjectTestOperations.checkExactMatchInTextEditorwithWhiteSpaces("#include <stdbool.h>\n" | ||
| + "#include <stdio.h>\n" + "#include <unistd.h>\n\n" + "void app_main(void) {\n" | ||
| + "while (true) {\n" + "printf(\"Hello from app_main!\\n\");\n" + "sleep(1);\n" + "}\n" + "}", | ||
| bot)); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Replace hardcoded sleep with auto-save completion wait
bot.sleep(1000) assumes the formatter & auto-save finish within 1 s.
Use a wait condition that polls the editor content until it matches the expected formatted text, or leverage waitForOperationsInProgressToFinishSync.
🤖 Prompt for AI Agents
In
tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/executable/cases/project/NewEspressifIDFProjectClangFilesTest.java
around lines 323 to 327, replace the hardcoded bot.sleep(1000) with a wait
mechanism that polls the editor content until it matches the expected formatted
text or use waitForOperationsInProgressToFinishSync to ensure the formatter and
auto-save operations complete before asserting. This avoids timing issues and
makes the test more reliable.
| ProjectTestOperations.launchCommandUsingContextMenu(projectName, bot, "Create Clangd Config"); | ||
| TestWidgetWaitUtility.waitForDialogToAppear(bot, "Clangd Configuration", 5000); | ||
| bot.shell("Clangd Configuration").bot().button("OK").click(); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Wait for asynchronous file creation after context-menu action
After clicking “OK” the .clangd file is created by background jobs.
Add an explicit wait (e.g. waitForTreeItem(".clangd", …) or waitForOperationsInProgressToFinishSync) before continuing; otherwise the next step may run before the file exists.
🤖 Prompt for AI Agents
In
tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/executable/cases/project/NewEspressifIDFProjectClangFilesTest.java
around lines 239 to 242, after clicking "OK" on the "Clangd Configuration"
dialog, the .clangd file is created asynchronously by background jobs. Add an
explicit wait after clicking "OK" to ensure the file creation completes before
proceeding. Use a method like waitForTreeItem(".clangd", ...) or
waitForOperationsInProgressToFinishSync to wait for the file to appear or for
background operations to finish.
|
@kolipakakondal @alirana01 @sigmaaa hi guys ! Please, review |
Description
Test cases:
Fixes # (IEP-XXX)
Type of change
Please delete options that are not relevant.
Test Configuration:
Checklist
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores