IEP 1553 adding unit tests for IDFUtil #1240
Conversation
WalkthroughA blank line was added after specific environment variable retrieval calls in the Changes
Sequence Diagram(s)sequenceDiagram
participant Test as IDFUtilTest
participant Util as IDFUtil
participant Env as IDFEnvironmentVariables
participant ToolChain as ESPToolChainManager
Test->>Util: getIDFPath()
Util->>Env: getEnvValue("IDF_PATH")
Env-->>Util: Return path
Util-->>Test: Return File
Test->>Util: getXtensaToolchainExecutablePathByTarget(target)
Util->>ToolChain: getToolchainExecutablePath(target)
ToolChain-->>Util: Return path or null
Util-->>Test: Return path or null
Test->>Util: checkIfIdfSupportsSpaces()
Util->>Util: getIdfVersion()
Util-->>Test: Return boolean
Test->>Util: getIDFExtraPaths()
Util->>Env: getEnvValue("IDF_PATH")
Env-->>Util: Return path
Util-->>Test: Return String
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: 5
🧹 Nitpick comments (1)
bundles/com.espressif.idf.core/src/com/espressif/idf/core/util/IDFUtil.java (1)
69-71:systemEnvPathProvideris defined but never usedDead code increases maintenance cost and annotation imports.
If future usage isn’t planned, remove it.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
bundles/com.espressif.idf.core/META-INF/MANIFEST.MF(1 hunks)bundles/com.espressif.idf.core/src/com/espressif/idf/core/util/IDFUtil.java(7 hunks)tests/com.espressif.idf.core.test/src/com/espressif/idf/core/util/test/IDFUtilTest.java(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
tests/com.espressif.idf.core.test/src/com/espressif/idf/core/util/test/IDFUtilTest.java (1)
bundles/com.espressif.idf.core/src/com/espressif/idf/core/util/IDFUtil.java (1)
IDFUtil(62-894)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build_macos
🔇 Additional comments (1)
bundles/com.espressif.idf.core/META-INF/MANIFEST.MF (1)
66-70: Version-range may not match the Guava bundle actually shipped in the IDE
com.google.common.annotationsis exported by the Guava bundle, not by a standalone package.
Declaring[33.4.0,34.0.0)forces the resolver to pick a very recent Guava (33.4+), while most Eclipse distributions still ship 32.x. Unless the target platform has already been upgraded, this import will make the bundle unresolved at run time.Either:
-Import-Package: com.google.common.annotations;version="[33.4.0,34.0.0)", +Import-Package: com.google.common.annotations;version="32.0.0"or declare an explicit dependency on the matching
com.google.guavabundle with a looser range (e.g."[30.0.0,35.0.0)").
Please verify the platform BOM before merging.
bundles/com.espressif.idf.core/src/com/espressif/idf/core/util/IDFUtil.java
Outdated
Show resolved
Hide resolved
bundles/com.espressif.idf.core/src/com/espressif/idf/core/util/IDFUtil.java
Outdated
Show resolved
Hide resolved
tests/com.espressif.idf.core.test/src/com/espressif/idf/core/util/test/IDFUtilTest.java
Show resolved
Hide resolved
tests/com.espressif.idf.core.test/src/com/espressif/idf/core/util/test/IDFUtilTest.java
Show resolved
Hide resolved
tests/com.espressif.idf.core.test/src/com/espressif/idf/core/util/test/IDFUtilTest.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
tests/com.espressif.idf.core.test/src/com/espressif/idf/core/util/test/IDFUtilTest.java (4)
41-44: Static provider mutation is never rolled-back – tests are order-dependent
IDFUtil.idfEnvVarPathProvideris overwritten here but never restored, so any later test that depends on the default provider will silently inherit"esp_idf_path". This violates test isolation and was already raised in a previous review.
Persist with the earlier advice: capture the original provider in@BeforeEachand restore it in@AfterEach, or introduce a helper such asIDFUtil.swapIdfEnvVarPathProvider(...).
45-60: Path comparison remains platform-fragile
assertEquals(expectedPath, result.getPath())will fail on Windows becauseresult.getPath()always contains forward slashes (generated byIDFUtil) whilePaths.get(...).toString()uses the platform separator. Normalise both sides or comparePathobjects. This issue has been pointed out before.Also applies to: 68-73, 80-85, 92-97
109-115:@AfterEachstill resets only the cache flagThe teardown clears
idfSupportsSpacesbut leaves the mutatedidfEnvVarPathProvider(and any other injected providers) intact, so leakage between tests is still possible. Please extend the teardown (or use the suggested swap helper) to restore all mutated static fields.
151-156: Replace Javaassertwith JUnit assertions for reliability
assert (firstCall);is skipped unless the JVM is started with-ea. UseassertTrue(firstCall)instead (same forsecondCall). This has been flagged previously.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/com.espressif.idf.core.test/src/com/espressif/idf/core/util/test/IDFUtilTest.java(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: build
- GitHub Check: build_macos
tests/com.espressif.idf.core.test/src/com/espressif/idf/core/util/test/IDFUtilTest.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
tests/com.espressif.idf.core.test/src/com/espressif/idf/core/util/test/IDFUtilTest.java (3)
45-50: 🛠️ Refactor suggestionPath comparison brittle on Windows – normalise or compare
Pathobjects
result.getPath()always uses'/', whereasPaths.get(...).toString()yields'\' on Windows.
All such direct-string comparisons will therefore fail on CI jobs that run on Windows.-assertEquals(expectedPath, result.getPath()); +assertEquals(Paths.get(expectedPath).normalize(), + result.toPath().normalize());Please apply the same normalisation (or switch to
Pathequality) in every test that asserts on file-system paths.
216-221:⚠️ Potential issueReplace plain Java
assertwith JUnit assertions
assert (firstCall);is ignored unless the JVM is started with-ea, rendering the test ineffective under most runners.-assert (firstCall); +assertTrue(firstCall, "First call should evaluate to true");Search & replace any further occurrences to keep the test suite reliable.
241-245: 🛠️ Refactor suggestionIncorrect PATH construction –
Path.append(":")creates invalid segmentsAppending
":"withorg.eclipse.core.runtime.Pathtreats the colon as a path segment, producing/:/, not a path-list separator.
Beside breaking on *nix, it is totally wrong on Windows.-String expected = new Path("esp_idf_path").append("components/esptool_py/esptool").append(":") - .append("esp_idf_path").append("components/espcoredump").append(":").append("esp_idf_path") - .append("components/partition_table").append(":").append("esp_idf_path").append("components/app_update") - .toString(); +String pathSep = File.pathSeparator; +String expected = String.join(pathSep, + "esp_idf_path/components/esptool_py/esptool", + "esp_idf_path/components/espcoredump", + "esp_idf_path/components/partition_table", + "esp_idf_path/components/app_update");Apply the same fix where similar construction is used.
🧹 Nitpick comments (1)
tests/com.espressif.idf.core.test/src/com/espressif/idf/core/util/test/IDFUtilTest.java (1)
175-181: Reflection to reset cache is fragile – expose a test-only reset helperUsing reflection to mutate private static state can break with future JDK
module restrictions and obscures intent.
Consider adding a package-privateIDFUtil.resetCachesForTest()and calling that
from@AfterEachinstead of reflective field writes.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/com.espressif.idf.core.test/src/com/espressif/idf/core/util/test/IDFUtilTest.java(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: build
- GitHub Check: build_macos
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
tests/com.espressif.idf.core.test/src/com/espressif/idf/core/util/test/IDFUtilTest.java (4)
52-57:⚠️ Potential issuePath comparison still OS-dependent → assertions will break on Windows
All these tests build
expectedPathviaPaths.get(...).toString()and then compare it withresult.getPath().
IDFUtilconcatenates segments with the forward-slash'/', so on Windows the RHS will contain'/'while the LHS contains'\\', causing false negatives.Normalize before comparing, e.g.:
-assertEquals(expectedPath, result.getPath()); +assertEquals( + Paths.get(expectedPath).toAbsolutePath().normalize(), + Paths.get(result.getPath()).toAbsolutePath().normalize());or compare
Pathobjects directly.Also applies to: 67-72, 93-98, 108-113, 135-138, 149-153, 162-166
248-252:⚠️ Potential issueExpected PATH string built with
Path.append(":")is invalidUsing Eclipse
Path.append(":")treats":"as a path segment, producing output likeesp_idf_path/:/….
Build the list withFile.pathSeparatoror plain':'(if *nix-only) and ordinary string concatenation instead.-String expected = new Path("esp_idf_path").append("components/esptool_py/esptool").append(":") - .append("esp_idf_path").append("components/espcoredump").append(":").append("esp_idf_path") - .append("components/partition_table").append(":").append("esp_idf_path").append("components/app_update") - .toString(); +String ps = File.pathSeparator; +String expected = String.join(ps, + "esp_idf_path/components/esptool_py/esptool", + "esp_idf_path/components/espcoredump", + "esp_idf_path/components/partition_table", + "esp_idf_path/components/app_update");
223-229:⚠️ Potential issueUse JUnit assertions instead of Java
assert
assert (firstCall);becomes a no-op unless the JVM runs with-ea.
Replace withassertTrue(firstCall)(and remove the parenthesis).
182-188:⚠️ Potential issueStatic lambda override not reset → hidden inter-test coupling
@AfterEachonly clearsidfSupportsSpaces; the overriddenidfEnvVarPathProvider
(assigned insideIDFUtil) persists across tests, so one test can influence another.Save the original provider in
@BeforeEachand restore it here, e.g.:private UnaryOperator<String> originalProvider; @BeforeEach void stash() { originalProvider = IDFUtil.swapIdfEnvVarPathProvider(original -> "esp_idf_path"); } @AfterEach void restore() throws Exception { IDFUtil.swapIdfEnvVarPathProvider(originalProvider); Field f = IDFUtil.class.getDeclaredField("idfSupportsSpaces"); f.setAccessible(true); f.set(null, null); }
🧹 Nitpick comments (1)
tests/com.espressif.idf.core.test/src/com/espressif/idf/core/util/test/IDFUtilTest.java (1)
45-73: Heavy duplication ofIDFEnvironmentVariablesmockingAlmost every test repeats the same
mockConstruction(IDFEnvironmentVariables…)boilerplate.
Extract a helper (or JUnit extension) to provide the mocked IDF path, reducing noise and maintenance overhead.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
tests/com.espressif.idf.core.test/META-INF/MANIFEST.MF(1 hunks)tests/com.espressif.idf.core.test/src/com/espressif/idf/core/util/test/IDFUtilTest.java(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- tests/com.espressif.idf.core.test/META-INF/MANIFEST.MF
⏰ 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 (2)
tests/com.espressif.idf.core.test/src/com/espressif/idf/core/util/test/IDFUtilTest.java (2)
223-228: Replace Javaassertwith JUnit assertion
assertstatements are ignored unless the JVM is started with-ea, so this test can silently succeed even when the logic is broken.boolean firstCall = IDFUtil.checkIfIdfSupportsSpaces(); -assert (firstCall); +assertTrue(firstCall);
248-252:Path.append(":")creates an invalid, OS-specific expectation
Path.appendtreats every argument as a path segment. Appending":"therefore yields something like.../:/..., which is never a real path list. Build the expected string with the platform path-separator instead:-String expected = new Path("esp_idf_path").append("components/esptool_py/esptool").append(":") - .append("esp_idf_path").append("components/espcoredump").append(":").append("esp_idf_path") - .append("components/partition_table").append(":").append("esp_idf_path").append("components/app_update") - .toString(); +String pathSep = File.pathSeparator; +String expected = String.join(pathSep, + "esp_idf_path/components/esptool_py/esptool", + "esp_idf_path/components/espcoredump", + "esp_idf_path/components/partition_table", + "esp_idf_path/components/app_update");
🧹 Nitpick comments (1)
tests/com.espressif.idf.core.test/src/com/espressif/idf/core/util/test/IDFUtilTest.java (1)
235-238: Nit: useSystem.lineSeparator()
System.lineSeparator()is the idiomatic way to fetch the platform line separator and is a touch clearer than reading the property explicitly.-String expected = System.getProperty("line.separator"); //$NON-NLS-1$ +String expected = System.lineSeparator();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/com.espressif.idf.core.test/src/com/espressif/idf/core/util/test/IDFUtilTest.java(1 hunks)
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
tests/com.espressif.idf.core.test/src/com/espressif/idf/core/util/test/IDFUtilTest.java (2)
226-232: Replace Javaassertwith JUnit assertions
assert (firstCall);relies on the JVM being started with-ea.
UseassertTrue(firstCall)(and similar) so the test is always executed.- boolean firstCall = IDFUtil.checkIfIdfSupportsSpaces(); - assert (firstCall); + boolean firstCall = IDFUtil.checkIfIdfSupportsSpaces(); + assertTrue(firstCall);
251-256: Expected PATH string is built incorrectly & is OS-specific
Path.append(":")appends":"as a path segment, producing/:/…on *nix and a back-slash mess on Windows.
Build the list withFile.pathSeparatorand plain strings instead.-String expected = new Path("esp_idf_path").append("components/esptool_py/esptool").append(":") - .append("esp_idf_path").append("components/espcoredump").append(":").append("esp_idf_path") - .append("components/partition_table").append(":").append("esp_idf_path").append("components/app_update") - .toString(); +String pathSep = File.pathSeparator; +String expected = String.join(pathSep, + "esp_idf_path/components/esptool_py/esptool", + "esp_idf_path/components/espcoredump", + "esp_idf_path/components/partition_table", + "esp_idf_path/components/app_update");
🧹 Nitpick comments (1)
tests/com.espressif.idf.core.test/src/com/espressif/idf/core/util/test/IDFUtilTest.java (1)
48-76: Heavy repetition of the sameIDFEnvironmentVariablesmockingEvery test re-creates identical
mockConstruction(IDFEnvironmentVariables …)boilerplate.
Consider extracting a small helper (e.g.withIdfPathMock("esp_idf_path", () -> { … })) or a JUnit extension to reduce noise and improve maintainability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/com.espressif.idf.core.test/src/com/espressif/idf/core/util/test/IDFUtilTest.java(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build_macos
sigmaaa
changed the title
Description
This PR introduces a comprehensive set of unit tests for the IDFUtil class. With the addition of these tests, the overall test coverage for this class (including UI-level tests) exceeds 80%.
Most of the added tests rely heavily on mocking to avoid significant refactoring at this stage. While mocking is not ideal for all scenarios, this approach provides a useful foundation. These tests can serve as a base for extending coverage or reproducing edge cases and bugs in the future.
Note: A few remaining methods are better suited for integration testing due to their system-level dependencies and side effects.
Fixes # (IEP-1553)
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