SWTBot test case: Clangd / Clang-Format Files Test #1131
Conversation
WalkthroughThis update introduces a comprehensive JUnit UI test class for Clangd and Clang-Format file workflows in Espressif IDF projects, generalizes a tab-waiting utility, adds new project operation methods, updates test logic for SDKconfig and file associations, raises the Java version for tests to 17, and disables a specific project rename/build test. Changes
Sequence Diagram(s)sequenceDiagram
participant Tester as Test Class
participant SWTBot as SWTWorkbenchBot
participant UI as Eclipse UI
participant Project as Espressif IDF Project
Tester->>SWTBot: Initialize environment/setup
SWTBot->>UI: Open project wizard
UI->>Project: Create new project
Tester->>SWTBot: Open/verify .clangd and .clang-format files
SWTBot->>UI: Open files in editor
Tester->>SWTBot: Edit/verify content, trigger formatting
SWTBot->>UI: Apply changes, trigger build
UI->>Project: Build and update project state
Tester->>SWTBot: Open preferences, verify Clangd arguments
SWTBot->>UI: Display and check preferences
Possibly related PRs
Suggested reviewers
Poem
✨ 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); | ||
| } | ||
| } | ||
| } No newline at end of file |
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.
2fe2261 to
05e74cd
Compare
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 |
alirana01
left a comment
alirana01
left a comment
There was a problem hiding this comment.
It looks good to me there is one minor thing that I think we can handle better. Instead of having static code lines inside our tests it would make sense to have some file in the resources to input and verify the formatting. It can help us save issues later and make it more configurable.
| prefrencesShell.bot().tree().getTreeItem("General").getNode("Editors").expand(); | ||
| prefrencesShell.bot().tree().getTreeItem("General").getNode("Editors").getNode("File Associations").select(); | ||
| prefrencesShell.bot().comboBox().setSelection("Text Editor"); | ||
| prefrencesShell.bot().comboBox().setSelection("Plain Text Editor"); |
There was a problem hiding this comment.
I am not so sure about this. Can't we just open the file in a plain text editor with swt bot without making it default for everything?
| 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)); | ||
| } |
There was a problem hiding this comment.
I am not sure about this approach.
@kolipakakondal @sigmaaa
Can you guys please weigh in on this? It would make sense to have these files and resources in project and then verify them against changes.
There was a problem hiding this comment.
Hi @alirana01,
What you’re saying makes perfect sense. Another option to improve readability is to use a multi-line text block. For example:
String code = """
#include <stdbool.h>
#include <stdio.h>
#include <unistd.h>
void app_main(void) {
while (true) {
printf("Hello from app_main!\\n");
sleep(1 );
}
}
""";
There was a problem hiding this comment.
@alirana01 , updated code with multi-line text block according to @sigmaaa comment.
I didn't get your point, do you mean, it is better to export this content to some separate file and then verify the actual values with pre-defined in this file ? If so, don't you think this approach is a bit overcomplicated, since we will need this file only once only in this particular Clangd test class ?
...om.espressif.idf.ui.test/src/com/espressif/idf/ui/test/operations/ProjectTestOperations.java
Outdated
Show resolved
Hide resolved
AndriiFilippov
force-pushed
the
IEP-1428
branch
2 times, most recently
from
c45b424 to
7d4c18f
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (27)
bundles/com.espressif.idf.branding/about.properties(1 hunks)bundles/com.espressif.idf.branding/about_zh.properties(1 hunks)bundles/com.espressif.idf.branding/plugin.properties(1 hunks)bundles/com.espressif.idf.debug.gdbjtag.openocd/src/com/espressif/idf/debug/gdbjtag/openocd/dsf/process/StreamListener.java(1 hunks)bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/handlers/Messages.java(1 hunks)bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/handlers/RenameIdfProjectParticipant.java(3 hunks)bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/handlers/messages.properties(1 hunks)releng/com.espressif.idf.configuration/pom.xml(1 hunks)releng/com.espressif.idf.target/com.espressif.idf.target.target(1 hunks)tests/com.espressif.idf.core.test/.classpath(0 hunks)tests/com.espressif.idf.core.test/META-INF/MANIFEST.MF(2 hunks)tests/com.espressif.idf.core.test/build.properties(0 hunks)tests/com.espressif.idf.core.test/src/com/espressif/idf/core/actions/test/ApplyTargetJobTest.java(3 hunks)tests/com.espressif.idf.core.test/src/com/espressif/idf/core/build/test/EspIdfErrorParserTest.java(4 hunks)tests/com.espressif.idf.core.test/src/com/espressif/idf/core/test/ActiveLaunchConfigurationTest.java(2 hunks)tests/com.espressif.idf.core.test/src/com/espressif/idf/core/test/DefaultBoardProviderTest.java(2 hunks)tests/com.espressif.idf.core.test/src/com/espressif/idf/core/test/ProcessBuilderFactoryTest.java(2 hunks)tests/com.espressif.idf.core.test/src/com/espressif/idf/core/test/ZipUtilityTest.java(1 hunks)tests/com.espressif.idf.core.test/src/com/espressif/idf/core/util/test/HintsUtilTest.java(1 hunks)tests/com.espressif.idf.core.test/src/com/espressif/idf/core/util/test/LaunchBarNameUtilTest.java(1 hunks)tests/com.espressif.idf.core.test/src/com/espressif/idf/core/util/test/LaunchConfigFinderTest.java(3 hunks)tests/com.espressif.idf.ui.test/META-INF/MANIFEST.MF(1 hunks)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(6 hunks)tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/operations/EnvSetupOperations.java(1 hunks)tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/operations/ProjectTestOperations.java(5 hunks)
💤 Files with no reviewable changes (2)
- tests/com.espressif.idf.core.test/build.properties
- tests/com.espressif.idf.core.test/.classpath
✅ Files skipped from review due to trivial changes (16)
- bundles/com.espressif.idf.branding/about.properties
- tests/com.espressif.idf.core.test/src/com/espressif/idf/core/util/test/HintsUtilTest.java
- bundles/com.espressif.idf.branding/about_zh.properties
- tests/com.espressif.idf.core.test/src/com/espressif/idf/core/test/ProcessBuilderFactoryTest.java
- tests/com.espressif.idf.core.test/src/com/espressif/idf/core/util/test/LaunchBarNameUtilTest.java
- releng/com.espressif.idf.configuration/pom.xml
- tests/com.espressif.idf.core.test/src/com/espressif/idf/core/build/test/EspIdfErrorParserTest.java
- tests/com.espressif.idf.core.test/src/com/espressif/idf/core/test/ZipUtilityTest.java
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/handlers/Messages.java
- tests/com.espressif.idf.core.test/src/com/espressif/idf/core/util/test/LaunchConfigFinderTest.java
- releng/com.espressif.idf.target/com.espressif.idf.target.target
- bundles/com.espressif.idf.debug.gdbjtag.openocd/src/com/espressif/idf/debug/gdbjtag/openocd/dsf/process/StreamListener.java
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/handlers/messages.properties
- tests/com.espressif.idf.core.test/src/com/espressif/idf/core/actions/test/ApplyTargetJobTest.java
- bundles/com.espressif.idf.branding/plugin.properties
- tests/com.espressif.idf.core.test/src/com/espressif/idf/core/test/DefaultBoardProviderTest.java
🚧 Files skipped from review as they are similar to previous changes (6)
- tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/operations/EnvSetupOperations.java
- tests/com.espressif.idf.ui.test/META-INF/MANIFEST.MF
- 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
- tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/executable/cases/project/NewEspressifIDFProjectClangFilesTest.java
🔇 Additional comments (8)
tests/com.espressif.idf.core.test/src/com/espressif/idf/core/test/ActiveLaunchConfigurationTest.java (2)
7-7: LGTM! Excellent JUnit 4 to JUnit 5 migration.The import update from
org.junit.Assert.assertEqualstoorg.junit.jupiter.api.Assertions.assertEqualsis the correct approach for modernizing to JUnit 5 while maintaining the same assertion functionality.
43-44: Clean formatting improvement.The whitespace cleanup before the assertion improves code consistency and readability.
tests/com.espressif.idf.core.test/META-INF/MANIFEST.MF (2)
69-72: Clean package export and import cleanup.The removal of JMock-related exports and JUnit Jupiter parameterized imports streamlines the manifest and reflects the modernized testing approach.
12-14:Details
✅ Verification successful
Excellent dependency modernization!
The addition of
org.mockito.mockito-coreand reordering of dependencies represents a good modernization from JMock to Mockito. This aligns well with the JUnit 4 to JUnit 5 migration effort.Please verify that all test classes can successfully resolve their dependencies after this change:
🏁 Script executed:
#!/bin/bash # Description: Verify that test files can find their required imports after dependency changes # Check for any remaining JUnit 4 imports that might need updating rg -A 2 "import.*org\.junit\.Assert" tests/ # Check for any remaining JMock imports that might need updating rg -A 2 "import.*org\.jmock" tests/ # Verify Mockito imports are being used correctly rg -A 2 "import.*org\.mockito" tests/Length of output: 7251
All core tests successfully migrated to Mockito and JUnit 5
The verification script confirms that in
tests/com.espressif.idf.core.test:
- There are no remaining
org.junit.Assertororg.jmockimports- All test classes use
org.mockitoimports as expectedDependency modernization in this module is complete—no further changes required.
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/handlers/RenameIdfProjectParticipant.java (4)
3-5: LGTM! Imports are appropriate for the new functionality.The added imports support the new build folder update functionality correctly.
Also applies to: 9-9, 18-18, 21-21
83-86: LGTM! Modern Java pattern matching syntax.The use of pattern matching for instanceof checks is a clean improvement and follows modern Java practices.
120-120: LGTM! Proper integration with Eclipse refactoring framework.Returning the UpdateBuildFolderChange instance instead of null properly integrates the build folder update into the refactoring lifecycle.
107-118:Details
✅ Verification successful
Review the syncExec workaround for potential threading issues.
The comment indicates this is a workaround, and the syncExec call could potentially cause UI freezing or deadlocks.
This workaround for saving/restoring the active launch target uses syncExec which could be problematic. Consider if this logic could be moved to asyncExec or handled differently to avoid potential UI blocking.
🏁 Script executed:
#!/bin/bash # Search for similar syncExec patterns in the codebase to understand if this is a common pattern rg -A 5 -B 5 "syncExec.*addDisposeListener"Length of output: 1639
🏁 Script executed:
#!/bin/bash sed -n '1,200p' bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/handlers/RenameIdfProjectParticipant.javaLength of output: 3567
No change required: syncExec usage is safe here
The call to Display.getDefault().syncExec only blocks the refactoring (non-UI) thread while immediately registering the dispose listener on the SWT UI thread. This does not deadlock or freeze the UI, and it guarantees the listener is attached before the shell is disposed.
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/handlers/RenameIdfProjectParticipant.java
Show resolved
Hide resolved
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/handlers/RenameIdfProjectParticipant.java
Show resolved
Hide resolved
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/handlers/RenameIdfProjectParticipant.java
Show resolved
Hide resolved
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/handlers/RenameIdfProjectParticipant.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (4)
tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/executable/cases/project/NewEspressifIDFProjectClangFilesTest.java (4)
71-71: Rename test method to match its actual functionality.The method name suggests it tests "ClangFilesPresenceAndContent" but the implementation validates both Clangd and Clang-Format files existence and content, not a generic "workflow".
-public void givenNewProjectIsCreatedThenTestClangFilesPresenceAndContentForNewProject() throws Exception +public void testClangdAndClangFormatFilesExistWithCorrectContent() throws Exception
216-216:⚠️ Potential issueAdd null safety checks for tree item access.
Direct calls to
getNode()can throwNullPointerExceptionif the tree item doesn't exist. Add proper null checks.-assertTrue(bot.tree().getTreeItem(projectName).getNode(".clangd") != null); +SWTBotTreeItem projectItem = bot.tree().getTreeItem(projectName); +assertNotNull("Project not found: " + projectName, projectItem); +assertTrue(".clangd file not found", projectItem.getNodes().contains(".clangd"));Apply similar changes to lines 221 and 226-227.
Also applies to: 221-221, 226-227
241-242: 🛠️ Refactor suggestionAdd wait for asynchronous file creation.
After clicking "OK", the
.clangdfile is created asynchronously. Add an explicit wait to ensure file creation completes.bot.shell("Clangd Configuration").bot().button("OK").click(); +TestWidgetWaitUtility.waitForOperationsInProgressToFinishSync(bot);
339-351: 🛠️ Refactor suggestionReplace sleep with dynamic wait for formatting completion.
The hardcoded sleep assumes formatting completes within 1 second. Use a dynamic wait that polls for the expected formatted content.
-bot.sleep(1000); -assertTrue(ProjectTestOperations.checkExactMatchInTextEditorwithWhiteSpaces(""" +// Wait for formatting to complete by polling the editor content +bot.waitUntil(new DefaultCondition() { + @Override + public boolean test() throws Exception { + return ProjectTestOperations.checkExactMatchInTextEditorwithWhiteSpaces(""" #include <stdbool.h> #include <stdio.h> #include <unistd.h> void app_main(void) { while (true) { printf("Hello from app_main!\\n"); sleep(1); } } -""", bot)); +""", bot); + } + + @Override + public String getFailureMessage() { + return "Formatting did not complete within timeout"; + } +}, 5000);
🧹 Nitpick comments (3)
tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/executable/cases/project/NewEspressifIDFProjectClangFilesTest.java (3)
234-234: Improve readability of negative assertion.Using negation with
contains()can be confusing. Consider usingassertFalsefor clarity.-assertTrue(!bot.tree().getTreeItem(projectName).getNodes().contains(".clangd")); +assertFalse(".clangd file should not exist", + bot.tree().getTreeItem(projectName).getNodes().contains(".clangd"));
263-265: Improve exception handling clarity.Instead of wrapping
CoreExceptioninIOException, consider letting the original exception propagate or use a more appropriate exception type.-} catch (CoreException e) { - throw new IOException("Failed to get build directory for project: " + projectName, e); +} catch (CoreException e) { + throw new RuntimeException("Failed to get build directory for project: " + projectName, e); }
229-229: Extract timeout values to constants.Magic numbers for timeouts are scattered throughout the code. Extract them to named constants for better maintainability.
Add these constants at the class level:
private static final int SHORT_TIMEOUT_MS = 1000; private static final int MEDIUM_TIMEOUT_MS = 5000; private static final int LONG_TIMEOUT_MS = 10000;Then replace the hardcoded values throughout the code.
Also applies to: 240-240, 247-247, 438-438, 450-450, 454-454, 457-457
📜 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/executable/cases/project/NewEspressifIDFProjectTest.java(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/executable/cases/project/NewEspressifIDFProjectTest.java
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build_macos
...com/espressif/idf/ui/test/executable/cases/project/NewEspressifIDFProjectClangFilesTest.java
Show resolved
Hide resolved
...com/espressif/idf/ui/test/executable/cases/project/NewEspressifIDFProjectClangFilesTest.java
Show resolved
Hide resolved
...com/espressif/idf/ui/test/executable/cases/project/NewEspressifIDFProjectClangFilesTest.java
Show resolved
Hide resolved
...com/espressif/idf/ui/test/executable/cases/project/NewEspressifIDFProjectClangFilesTest.java
Show resolved
Hide resolved
...com/espressif/idf/ui/test/executable/cases/project/NewEspressifIDFProjectClangFilesTest.java
Show resolved
Hide resolved
...com/espressif/idf/ui/test/executable/cases/project/NewEspressifIDFProjectClangFilesTest.java
Show resolved
Hide resolved
AndriiFilippov
force-pushed
the
IEP-1428
branch
2 times, most recently
from
c45b424 to
4353076
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/executable/cases/project/NewEspressifIDFProjectSDKconfigTest.java (1)
139-142: Consider removing redundant helper method.The
refreshProjectInProjectExplorer()method is only used once and simply wraps a direct call toProjectTestOperations.launchCommandUsingContextMenu(). This creates unnecessary indirection without adding value.Consider removing this method and using the direct call:
- Fixture.refreshProjectInProjectExplorer(); + ProjectTestOperations.launchCommandUsingContextMenu(projectName, bot, "Refresh");And remove the helper method:
- private static void refreshProjectInProjectExplorer () throws Exception - { - ProjectTestOperations.launchCommandUsingContextMenu(projectName, bot, "Refresh"); - }
📜 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/NewEspressifIDFProjectSDKconfigTest.java(6 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
- GitHub Check: build_macos
🔇 Additional comments (4)
tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/executable/cases/project/NewEspressifIDFProjectSDKconfigTest.java (4)
56-57: Enhanced test workflow with project build operation.Adding the project build step immediately after project creation improves test coverage by ensuring the project is properly built before testing SDK configuration workflows. The refresh operation ensures the UI reflects the build artifacts.
71-71: Consistent project build step added.The addition of the build step maintains consistency with the other test method and ensures proper project setup before SDK configuration testing.
134-134: Project refresh after build operation.Adding the refresh command after building the project ensures the Project Explorer view is updated with the newly generated SDK configuration file. This is a good practice for UI tests.
187-187: Improved wait utility method usage.The update from
waitForSDKConfigurationTab()towaitForCTabToAppear()with explicit tab name parameter makes the code more maintainable and aligns with the generalized utility method approach mentioned in the AI summary.Also applies to: 194-194
.../com/espressif/idf/ui/test/executable/cases/project/NewEspressifIDFProjectSDKconfigTest.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (7)
tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/executable/cases/project/NewEspressifIDFProjectClangFilesTest.java (7)
258-266: Fix indentation inconsistency in try-catch block.The indentation inside the try-catch block is inconsistent with the rest of the codebase.
private static String getExpectedBuildFolderPATH(String projectName) throws IOException { - try { - IProject project = ResourcesPlugin.getWorkspace().getRoot().getProject(projectName); - String buildFolder = IDFUtil.getBuildDir(project); - Path buildFolderPath = Paths.get(buildFolder); - return 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 buildFolderPath.toAbsolutePath().toString(); + } + catch (CoreException e) + { + throw new IOException("Failed to get build directory for project: " + projectName, e); + } }
331-331: Simplify string escaping in text block.The
\\nescape sequence is unnecessary within a text block literal. Use a simple\ninstead.- printf("Hello from app_main!\\n"); + printf("Hello from app_main!\n");
396-404: Fix indentation inconsistency in try-catch block.Similar to the previous method, the indentation inside this try-catch block is inconsistent.
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); + } }
414-418: Fix indentation and improve formatting consistency.The method has inconsistent indentation and spacing.
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(expectedClangdPath.equals(actualClangdPath)); + }
421-427: Fix indentation and improve formatting consistency.Similar formatting issues as the previous method.
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(expectedClangdPath.equals(actualClangdPath)); + }
447-451: Extract timeout constants for better maintainability.The hardcoded timeout values (10000) are repeated and should be extracted as constants for better maintainability.
+ private static final int DIALOG_TIMEOUT_MS = 10000; + private static void closePreferencesDialog() { SWTBotShell preferencesShell = bot.shell("Preferences"); preferencesShell.bot().button("Cancel").click(); // Or "Apply and Close" - TestWidgetWaitUtility.waitWhileDialogIsVisible(bot, "Preferences", 10000); + TestWidgetWaitUtility.waitWhileDialogIsVisible(bot, "Preferences", DIALOG_TIMEOUT_MS); } private static void closeTargetDialog() { - TestWidgetWaitUtility.waitForDialogToAppear(bot, "IDF Launch Target Changed", 10000); + TestWidgetWaitUtility.waitForDialogToAppear(bot, "IDF Launch Target Changed", DIALOG_TIMEOUT_MS); SWTBotShell preferencesShell = bot.shell("IDF Launch Target Changed"); preferencesShell.bot().button("Yes").click(); // Or "Apply and Close" - TestWidgetWaitUtility.waitWhileDialogIsVisible(bot, "IDF Launch Target Changed", 10000); + TestWidgetWaitUtility.waitWhileDialogIsVisible(bot, "IDF Launch Target Changed", DIALOG_TIMEOUT_MS); }Also applies to: 453-458
271-273: Add descriptive error messages to assertions.The assertions lack descriptive error messages, making test failures harder to debug.
private static void thenClangdFileContentChecked() throws Exception { bot.cTabItem(".clangd").activate(); - assertTrue(ProjectTestOperations.checkExactMatchInTextEditor( - "CompileFlags:\n" + " CompilationDatabase: build\n" + " Remove: [-m*, -f*]", bot)); + assertTrue("Clangd file content does not match expected format", + ProjectTestOperations.checkExactMatchInTextEditor( + "CompileFlags:\n" + " CompilationDatabase: build\n" + " Remove: [-m*, -f*]", bot)); } private static void thenClangFormatContentChecked() throws Exception { bot.cTabItem(".clang-format").activate(); - assertTrue(ProjectTestOperations.checkExactMatchInTextEditor( + assertTrue("Clang-format file content does not match expected format", + 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. BasedOnStyle: LLVM UseTab: Always IndentWidth: 4 TabWidth: 4 BreakConstructorInitializers: AfterColon IndentAccessModifiers: false AccessModifierOffset: -4 """, bot)); }Also applies to: 291-302
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
tests/com.espressif.idf.ui.test/META-INF/MANIFEST.MF(1 hunks)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(6 hunks)tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/executable/cases/project/NewEspressifIDFProjectTest.java(1 hunks)tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/operations/EnvSetupOperations.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 (6)
- tests/com.espressif.idf.ui.test/META-INF/MANIFEST.MF
- tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/operations/EnvSetupOperations.java
- tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/executable/cases/project/NewEspressifIDFProjectTest.java
- 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
- GitHub Check: build_macos
...com/espressif/idf/ui/test/executable/cases/project/NewEspressifIDFProjectClangFilesTest.java
Show resolved
Hide resolved
| Bundle-SymbolicName: com.espressif.idf.ui.test;singleton:=true | ||
| Bundle-Version: 0.0.1.qualifier | ||
| Bundle-RequiredExecutionEnvironment: JavaSE-11 | ||
| Bundle-RequiredExecutionEnvironment: JavaSE-17 |
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