#2312: Fix NPE in Uv.setEnvironment and suppress errors during auto-c… - #2332
#2312: Fix NPE in Uv.setEnvironment and suppress errors during auto-c…#2332krystynaShatkovska wants to merge 6 commits into
Conversation
Coverage Report for CI Build 32224414199Coverage decreased (-0.007%) to 72.929%Details
Uncovered ChangesNo uncovered changes found. Coverage Regressions83 previously-covered lines in 3 files lost coverage.
Coverage Stats💛 - Coveralls |
There was a problem hiding this comment.
Great Approach on fixing the issue! I've followed your testing instructions and it works. I have a few suggestions and questions though:
- You should add tests for the bugs mentioned in the issue, that fail before the fix and run fine with the fix. (test 1: Uv.setEnvironment() does not throw when getSoftwarePath() is null. test 2: when a processable-output commandlet throws inside run(), no ERROR-level text and no "Logfile can be found at ..." line leak into the captured log buffer, but the step is still marked as failed.)
- Your first commit message contains "Co-Authored-By: Capgemini Sovereign AI Platform", this is violating the CLA and needs to be removed.
- Your second commit message for the Fix is missing the "#2312:" prefix.
- When i run
ide install python invalid-version-xyzin/tmpi get the following Message:You are not inside an IDE project: C:\Users\...\AppData\Local\Temp Error: IDEasy failed with exit code 2, is this the error message that should be written?
…ommandlets The original fix wrapped both activateLogging() and step.error() inside the isProcessableOutput check. However, step.error(t, true) logs at DEBUG level (not ERROR) and is needed for proper step tracking and test assertions. Only activateLogging() should be skipped to prevent errors from appearing in the terminal during auto-completion.
… auto-completion Address the two bugs reported on devonfw#2312 and add regression tests for both. 1) NPE in Uv.setEnvironment when the software path is null. - Guard the software-path resolution with a null check (same idiom as LocalToolCommandlet.getToolPath). - Add UvTest#testSetEnvironmentWithNullSoftwarePath: a genuine regression test that throws a NullPointerException before the fix and passes after. 2) Error leak during auto-completion / processable output. - When a processable-output commandlet (e.g. complete) throws, the previous code rethrew the exception, which made Ideasy.run() log the 'An unexpected error occurred! ... please file a bug' block at ERROR level into the machine-consumed output. - Rework the catch block in AbstractIdeContext.run(): for a processable-output commandlet, record the failure via step.error(t, true) (still marks the step as failed) and fail quietly instead of rethrowing, so no ERROR block or 'Logfile can be found at ...' line is emitted into the captured output. - Add IdeasyTest#testProcessableOutputCommandletFailureDoesNotLogError: a regression test that leaks the ERROR block before the fix and passes after. Verified: full 'mvn clean test' passes (all modules, 0 failures). Both new tests fail against the pre-fix behavior and pass with the fix.
50c59f4 to
3aa30e4
Compare
samuelkos17
left a comment
There was a problem hiding this comment.
Thanks for adding the tests and applying my suggestions! I've actually made a mistake on my suggestion regarding the if/else branch, but you corrected it and made a much cleaner implementation, good job! 😄 I've also tested the tests and can confirm that they fail on main and pass with your bug fix. Also thank you for clarifying my question regarding the error code, I just wanted to make sure that it works as intended and that it wont be missed. I just have a small suggestion now for one of your tests, since you cleaned up the if/else branch and make it return 1 you can also assert that it will return 1.
| // assert - the step is marked as failed | ||
| assertThat(context).logAtDebug().hasMessage("Step 'ide' ended with failure."); | ||
| // assert - no ERROR-level error block and no "Logfile can be found at" line leaked into the captured output | ||
| assertThat(exitCode).isNotEqualTo(0); |
There was a problem hiding this comment.
| assertThat(exitCode).isNotEqualTo(0); | |
| assertThat(exitCode).isEqualTo(1); |
This PR fixes #2312
Implemented changes:
Uv.java - Added a null check on getSoftwarePath(). If it is null, setEnvironment() returns early instead of crashing.
AbstractIdeContext.java - In the catch (Throwable) block, exceptions during processable-output commandlets (like CompleteCommandlet) are no longer logged to stderr. The exception
still re-throws, but nothing reaches the user's screen, keeping auto-completion output clean.
Testing instructions
Test 1: Auto-completion outside an IDEasy project
Navigate to a directory that is NOT inside an IDEasy project
cd /tmp
Trigger auto-completion for Python versions (no IDE_HOME)
ide complete install python ""
Expected: Clean output with version suggestions only, no error messages, no NPE.
Before fix: NullPointerException error printed to console.
Test 2: Auto-completion inside an IDEasy project
Navigate to an IDEasy project
cd
ide complete install python ""
Expected: Clean output with version suggestions only.
Test 3: Normal error display for regular commands
cd /tmp
ide install python invalid-version-xyz
Expected: Error IS displayed normally (the user should see it for real commands, only auto-completion suppresses errors).
Test 4: Uv.setEnvironment with no IDE_HOME
cd /tmp
This should not crash even if it triggers uv environment setup
ide complete install python ""
Expected: No crash, clean output.
Checklist for this PR
mvn clean testlocally all tests pass and build is successful#«issue-id»: «brief summary»(e.g.#921: fixed setup.batand notfeature/921 fixed setup.bat). If no issue ID exists, title only.In Progressand assigned to you or there is no issue (might happen for very small PRs)with
internalpom.xmlfiles or otherwise if runtime dependencies changed, you have updated our LICENSE.asciidoc