IEP-1767 shouldHaveClangFilesPresentAndContentCorrectForNewProject test is not stable#1461
Merged
Conversation
📝 WalkthroughWalkthroughRefactors NewEspressifIDFProjectClangFilesTest to use assertEquals and a new assertTextEqualsNormalized helper, replacing helper-based editor checks with direct active-editor text comparisons and explicit boolean presence assertions for ChangesClang Files Test Assertion Refactoring
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/executable/cases/project/NewEspressifIDFProjectClangFilesTest.java (3)
251-252: 💤 Low valueConsider whether
trim()might mask whitespace issues.Applying
trim()to both expected and actual text makes the test more lenient but may hide legitimate formatting problems. If the actual.clang-formatfile has unexpected trailing whitespace or newlines, this test won't detect it. Since.clang-formatis a YAML configuration file where whitespace can matter, consider whether this tolerance is appropriate or if exact matching (including trailing whitespace) would catch more issues.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/executable/cases/project/NewEspressifIDFProjectClangFilesTest.java` around lines 251 - 252, The test in NewEspressifIDFProjectClangFilesTest currently uses assertEquals("ClangFormat content did not match", expectedText.trim(), actualText.trim()), which may hide legitimate trailing-whitespace/newline differences in the .clang-format YAML; change the assertion to compare exact content (remove trim() calls) so it asserts expectedText.equals(actualText) via assertEquals("ClangFormat content did not match", expectedText, actualText), or alternatively normalize only line endings (e.g., replace "\r\n" with "\n") but do not strip trailing whitespace/newlines before asserting to ensure true formatting differences are detected.
157-158: ⚡ Quick winPrefer
assertTrue/assertFalsefor boolean assertions.Using
assertEquals(true/false, boolean)doesn't provide better failure diagnostics thanassertTrue/assertFalsefor boolean values. The benefit ofassertEqualsfor richer diffs applies to complex objects and strings, not to simple boolean comparisons where the output is just "expected: true, actual: false".♻️ Revert to idiomatic boolean assertions
- boolean isPresent = bot.tree().getTreeItem(projectName).getNodes().contains(".clangd"); - assertEquals("The .clangd file should be present", true, isPresent); + assertTrue("The .clangd file should be present", + bot.tree().getTreeItem(projectName).getNodes().contains(".clangd"));- boolean isPresent = bot.tree().getTreeItem(projectName).getNodes().contains(".clang-format"); - assertEquals("The .clang-format file should be present", true, isPresent); + assertTrue("The .clang-format file should be present", + bot.tree().getTreeItem(projectName).getNodes().contains(".clang-format"));- boolean isPresent = bot.tree().getTreeItem(projectName).getNodes().contains(".clangd"); - assertEquals("The .clangd file should be absent", false, isPresent); + assertFalse("The .clangd file should be absent", + bot.tree().getTreeItem(projectName).getNodes().contains(".clangd"));Also applies to: 163-164, 177-178
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/executable/cases/project/NewEspressifIDFProjectClangFilesTest.java` around lines 157 - 158, Replace boolean equality assertions using assertEquals with the idiomatic boolean assertions assertTrue/assertFalse: change the assertion that checks the boolean variable isPresent (in NewEspressifIDFProjectClangFilesTest, where you call bot.tree().getTreeItem(projectName).getNodes().contains(".clangd")) to assertTrue when expecting presence (and assertFalse for absence). Apply the same replacement for the other boolean checks referenced in the comment (the assertions around the same pattern at the other occurrences noted).
304-305: ⚡ Quick winUsing
trim()in a formatting verification test may defeat its purpose.This test validates that Clang-Format correctly applied formatting settings. Using
trim()on both expected and actual text means trailing whitespace differences won't be detected. If Clang-Format produces unexpected trailing spaces or newlines, this test won't catch it. Consider whether exact byte-for-byte matching (withouttrim()) would provide stronger validation of the formatting behavior.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/executable/cases/project/NewEspressifIDFProjectClangFilesTest.java` around lines 304 - 305, The test NewEspressifIDFProjectClangFilesTest currently uses assertEquals("Formatted main file content did not match", expectedText.trim(), actualText.trim()); which hides trailing whitespace/newline differences; update the assertion to compare expectedText and actualText exactly (remove trim()) so the test asserts byte-for-byte equality, or if line-ending normalization is required, normalize both strings to a consistent newline sequence first and then assert exact equality using the same assertEquals call.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/executable/cases/project/NewEspressifIDFProjectClangFilesTest.java`:
- Around line 218-221: The test uses direct assertEquals on
bot.activeEditor().toTextEditor().getText() which skips the prior
line-ending/whitespace normalization and will fail on Windows; restore
normalization by either calling the existing helper
checkExactMatchInTextEditor(...) or normalize actualText before asserting
(replace CRLF "\r\n" with "\n" and trim trailing spaces on each line) so
NewEspressifIDFProjectClangFilesTest compares platform-independent text content;
update both assertion sites (the assertEquals at the getText() usage and the
similar one at the later lines) to use the same normalization helper or inline
normalization.
---
Nitpick comments:
In
`@tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/executable/cases/project/NewEspressifIDFProjectClangFilesTest.java`:
- Around line 251-252: The test in NewEspressifIDFProjectClangFilesTest
currently uses assertEquals("ClangFormat content did not match",
expectedText.trim(), actualText.trim()), which may hide legitimate
trailing-whitespace/newline differences in the .clang-format YAML; change the
assertion to compare exact content (remove trim() calls) so it asserts
expectedText.equals(actualText) via assertEquals("ClangFormat content did not
match", expectedText, actualText), or alternatively normalize only line endings
(e.g., replace "\r\n" with "\n") but do not strip trailing whitespace/newlines
before asserting to ensure true formatting differences are detected.
- Around line 157-158: Replace boolean equality assertions using assertEquals
with the idiomatic boolean assertions assertTrue/assertFalse: change the
assertion that checks the boolean variable isPresent (in
NewEspressifIDFProjectClangFilesTest, where you call
bot.tree().getTreeItem(projectName).getNodes().contains(".clangd")) to
assertTrue when expecting presence (and assertFalse for absence). Apply the same
replacement for the other boolean checks referenced in the comment (the
assertions around the same pattern at the other occurrences noted).
- Around line 304-305: The test NewEspressifIDFProjectClangFilesTest currently
uses assertEquals("Formatted main file content did not match",
expectedText.trim(), actualText.trim()); which hides trailing whitespace/newline
differences; update the assertion to compare expectedText and actualText exactly
(remove trim()) so the test asserts byte-for-byte equality, or if line-ending
normalization is required, normalize both strings to a consistent newline
sequence first and then assert exact equality using the same assertEquals call.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5f73a6ed-e220-474c-94e3-ebf686f542cb
📒 Files selected for processing (1)
tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/executable/cases/project/NewEspressifIDFProjectClangFilesTest.java
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/executable/cases/project/NewEspressifIDFProjectClangFilesTest.java`:
- Line 69: The test calls Fixture.thenClangFormatContentChecked() immediately
after opening ".clangd" which is wrong because ".clang-format" isn't opened yet;
replace that invocation with Fixture.thenClangdContentChecked() so the check
verifies the opened ".clangd" buffer (method names:
thenClangFormatContentChecked -> thenClangdContentChecked, context:
NewEspressifIDFProjectClangFilesTest test that opens ".clangd" before checking
content) and keep the existing thenClangFormatContentChecked() call later where
".clang-format" is actually opened.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7e77c26c-4c0d-4816-a61f-05ba3dbb9bc2
📒 Files selected for processing (1)
tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/executable/cases/project/NewEspressifIDFProjectClangFilesTest.java
There was a problem hiding this comment.
🧹 Nitpick comments (4)
tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/executable/cases/project/NewEspressifIDFProjectClangFilesTest.java (4)
235-253: 💤 Low valueRemove redundant trim() calls.
The
assertTextEqualsNormalizedhelper already trims both strings (line 128-129), so the explicit.trim()calls on line 252 are redundant. The comment on line 251 suggests concern about text block processing, but the helper already handles this.♻️ Simplified version
- // Using trim() to avoid mismatch purely due to trailing spaces/newlines from text block processing - assertTextEqualsNormalized("ClangFormat content did not match", expectedText.trim(), actualText.trim()); + assertTextEqualsNormalized("ClangFormat content did not match", expectedText, actualText);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/executable/cases/project/NewEspressifIDFProjectClangFilesTest.java` around lines 235 - 253, The helper thenClangFormatContentChecked calls assertTextEqualsNormalized("ClangFormat content did not match", expectedText.trim(), actualText.trim()) but assertTextEqualsNormalized already normalizes/trims both inputs, so remove the redundant .trim() calls on expectedText and actualText in thenClangFormatContentChecked and update/keep the comment to reflect that normalization is handled by assertTextEqualsNormalized; locate the method name thenClangFormatContentChecked and the assertion call to make the change.
166-167: 💤 Low valueConsider using assertTrue for boolean presence checks.
While
assertEquals(true, isPresent)works correctly,assertTrue(isPresent)is more idiomatic for boolean assertions and provides the same diff quality. The benefit ofassertEqualsfor better diffs mainly applies to comparing complex objects or strings where seeing both values side-by-side is helpful.♻️ Alternative using assertTrue
- boolean isPresent = bot.tree().getTreeItem(projectName).getNodes().contains(".clangd"); - assertEquals("The .clangd file should be present", true, isPresent); + boolean isPresent = bot.tree().getTreeItem(projectName).getNodes().contains(".clangd"); + assertTrue("The .clangd file should be present", isPresent);And similarly for .clang-format:
- boolean isPresent = bot.tree().getTreeItem(projectName).getNodes().contains(".clang-format"); - assertEquals("The .clang-format file should be present", true, isPresent); + boolean isPresent = bot.tree().getTreeItem(projectName).getNodes().contains(".clang-format"); + assertTrue("The .clang-format file should be present", isPresent);Also applies to: 172-173
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/executable/cases/project/NewEspressifIDFProjectClangFilesTest.java` around lines 166 - 167, Replace the boolean equality assertions that use assertEquals(true, isPresent) in the NewEspressifIDFProjectClangFilesTest with assertTrue(isPresent); specifically update the assertion that validates the presence of the ".clangd" file (variable isPresent) to use assertTrue and do the same for the similar check that validates the ".clang-format" file to make the boolean checks more idiomatic.
288-307: 💤 Low valueRemove redundant trim() calls.
Similar to the clang-format check, the explicit
.trim()calls on lines 305-306 are redundant sinceassertTextEqualsNormalizedalready trims both strings.♻️ Simplified version
- assertTextEqualsNormalized("Formatted main file content did not match", expectedText.trim(), - actualText.trim()); + assertTextEqualsNormalized("Formatted main file content did not match", expectedText, actualText);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/executable/cases/project/NewEspressifIDFProjectClangFilesTest.java` around lines 288 - 307, The test method checkMainFileContentFormattedUnderActualSettings contains redundant .trim() calls on both expectedText and actualText when calling assertTextEqualsNormalized; remove the .trim() invocations so you pass the raw expectedText and actualText to assertTextEqualsNormalized (which already normalizes/trims inputs) and keep the rest of the method intact.
186-187: 💤 Low valueConsider using assertFalse for boolean absence checks.
Similar to the presence checks,
assertFalse(isPresent)is more idiomatic thanassertEquals(false, isPresent)for boolean assertions.♻️ Alternative using assertFalse
- boolean isPresent = bot.tree().getTreeItem(projectName).getNodes().contains(".clangd"); - assertEquals("The .clangd file should be absent", false, isPresent); + boolean isPresent = bot.tree().getTreeItem(projectName).getNodes().contains(".clangd"); + assertFalse("The .clangd file should be absent", isPresent);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/executable/cases/project/NewEspressifIDFProjectClangFilesTest.java` around lines 186 - 187, Replace the boolean equality assertion with an idiomatic boolean assertion: in NewEspressifIDFProjectClangFilesTest where you compute boolean isPresent = bot.tree().getTreeItem(projectName).getNodes().contains(".clangd"), change the assertion from assertEquals("The .clangd file should be absent", false, isPresent) to use assertFalse (either assertFalse("The .clangd file should be absent", isPresent) or assertFalse(isPresent)) so the test expresses absence more clearly and follows JUnit conventions.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In
`@tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/executable/cases/project/NewEspressifIDFProjectClangFilesTest.java`:
- Around line 235-253: The helper thenClangFormatContentChecked calls
assertTextEqualsNormalized("ClangFormat content did not match",
expectedText.trim(), actualText.trim()) but assertTextEqualsNormalized already
normalizes/trims both inputs, so remove the redundant .trim() calls on
expectedText and actualText in thenClangFormatContentChecked and update/keep the
comment to reflect that normalization is handled by assertTextEqualsNormalized;
locate the method name thenClangFormatContentChecked and the assertion call to
make the change.
- Around line 166-167: Replace the boolean equality assertions that use
assertEquals(true, isPresent) in the NewEspressifIDFProjectClangFilesTest with
assertTrue(isPresent); specifically update the assertion that validates the
presence of the ".clangd" file (variable isPresent) to use assertTrue and do the
same for the similar check that validates the ".clang-format" file to make the
boolean checks more idiomatic.
- Around line 288-307: The test method
checkMainFileContentFormattedUnderActualSettings contains redundant .trim()
calls on both expectedText and actualText when calling
assertTextEqualsNormalized; remove the .trim() invocations so you pass the raw
expectedText and actualText to assertTextEqualsNormalized (which already
normalizes/trims inputs) and keep the rest of the method intact.
- Around line 186-187: Replace the boolean equality assertion with an idiomatic
boolean assertion: in NewEspressifIDFProjectClangFilesTest where you compute
boolean isPresent =
bot.tree().getTreeItem(projectName).getNodes().contains(".clangd"), change the
assertion from assertEquals("The .clangd file should be absent", false,
isPresent) to use assertFalse (either assertFalse("The .clangd file should be
absent", isPresent) or assertFalse(isPresent)) so the test expresses absence
more clearly and follows JUnit conventions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0d6199bf-f630-4162-a450-165c79da9e24
📒 Files selected for processing (1)
tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/executable/cases/project/NewEspressifIDFProjectClangFilesTest.java
sigmaaa
changed the title
kolipakakondal
approved these changes
May 19, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
This PR contains a refactor of the test by replacing assertTrue with assertEquals for better diffs, as well as a test fix. The test was failing because we expected CompilationDatabase as the build value after creating a new project, but it is actually an absolute path now.
Fixes # (IEP-1767)
Type of change
Please delete options that are not relevant.
How has this been tested?
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration
Test Configuration:
Dependent components impacted by this PR:
Checklist
Summary by CodeRabbit