#2102 set ideRoot during msi installation correctly#2149
Conversation
Coverage Report for CI Build 29415537689Warning No base build found for commit Coverage: 72.281%Details
Uncovered ChangesNo uncovered changes found. Coverage RegressionsRequires a base build to compare against. How to fix this → Coverage Stats
💛 - Coveralls |
oanding-blrng
left a comment
There was a problem hiding this comment.
The provided testing instructions are working as expected, other tests are running successfully. I added four suggestion as part of my review.
Also, in case, that the behaviour mentioned in ticket #2102 negatively impacts the user, the testing instructions should be extended to include proper instructions for manually testing correct behaivour.
Co-authored-by: OAnding <oscar.anding@capgemini.com>
Co-authored-by: OAnding <oscar.anding@capgemini.com>
Co-authored-by: OAnding <oscar.anding@capgemini.com>
Co-authored-by: OAnding <oscar.anding@capgemini.com>
|
So, the issue does not negatively impact the user. At its core, it is a robustness and root-cause fix. The only actual user-visible symptom was issue #1517 |
Then the current testing instructions are fine. This ticket can move forward. |
This PR fixes #2102
Implemented changes:
AbstractIdeContext:ideRootis no longerfinal; addedsetIdeRoot(Path)so the context canreflect the derived install target.
IdeContext: addedsetIdeRoot(Path)to the interface.IdeasyCommandlet.installIdeasy(...): after deriving the target viadetermineIdeRoot(cwd), itnow calls
context.setIdeRoot(...), keeping the whole install run consistent.AbstractIdeTestContext: removed the now-redundantideRoot/ideRootSetfields andgetIdeRoot/setIdeRootoverrides the test context inherits the production setter, aligningtest and production behaviour.
IdeasyCommandletTest#testInstallIdeasyUpdatesIdeRoot(windows/mac/linux) that failswithout the fix and passes with it.
Verified the invariants called out in the issue are unaffected: the
isIdeRootRequiredgate inapplyAndRun, and the temp-download-path creation guarded byideRoot != nullin the constructor.Testing instructions
cli/, runmvn -Dtest=IdeasyCommandletTest testall cases pass, includingthe new
testInstallIdeasyUpdatesIdeRoot. (Reverting thecontext.setIdeRoot(...)line makes the3 new cases fail, confirming the test reproduces the bug.)
Checklist for this PR
Make sure everything is checked before merging this PR. For further info please also see
our DoD.
mvn clean testlocally all tests pass and build is successful#«issue-id»: «brief summary»(e.g.#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)internal