#1869: Improve windows helper to search windows registry#1876
#1869: Improve windows helper to search windows registry#1876jakozian wants to merge 6 commits intodevonfw:mainfrom
Conversation
… entries from registry
jakozian
added
internal
Coverage Report for CI Build 25105221975Coverage decreased (-0.04%) to 70.633%Details
Uncovered ChangesNo uncovered changes found. Coverage Regressions19 previously-covered lines in 3 files lost coverage.
Coverage Stats
💛 - Coveralls |
MarvMa
left a comment
MarvMa
left a comment
There was a problem hiding this comment.
Thats a really easy to read PR following the KISS principle, which I personally appreciate a lot 👍 I left some comments regarding minor changes. I have been more critical since that has been the desired task. But the coments are just related to minor changes in code-style regarding the coding-conventions of IDEasy.
|
|
||
| /** | ||
| * @param appName the application name to search for in the Windows registry. | ||
| * @return the DisplayName entry if the application is found in the Windows registry or {@code null} if nothing was found. |
There was a problem hiding this comment.
Nice addition of the lookup methods! The JavaDoc could be enriched with some more information regarding the search itself e.g. which paths are beeing searched and whether the matching is case insensitive, exact or substring match.
|
|
||
| for (String registryBasePath : REGISTRY_BASE_PATHS) { | ||
| List<String> out = runReg("query", registryBasePath, "/s", "/f", appName); | ||
| if (out != null) { |
There was a problem hiding this comment.
Here it could be worth considering adding a check for an empty list. the runReg() method in the test returns List.of() for a miss, while the actual implementation returns null. Updating the return for a miss in the test-method or add an empty list check here could solve these different "miss" values.
| if (out != null) { | |
| if (out != null && !out.isEmpty()) { |
| } | ||
|
|
||
| @Override | ||
| protected List<String> runReg(String... args) { |
There was a problem hiding this comment.
Overwriting the runReg in the Test is a clean style. One thing I found is that the simulation logic appears to be identical in WindowsHelperMock. To reduce redundancy the WindowsHelper could delegate to the WindowsHelperImplTestable method.
| } | ||
|
|
||
| // Only return output if searched app matches | ||
| if (!"TestApp".equalsIgnoreCase(searchValue)) { |
There was a problem hiding this comment.
You could reuse the already existing MOCK_APP_NAME constant here.
| if (!"TestApp".equalsIgnoreCase(searchValue)) { | |
| if (!MOCK_APP_NAME .equalsIgnoreCase(searchValue)) { |
| * Tests if correct keys can be found in registry output for app name filter. | ||
| */ | ||
| @Test | ||
| void testRegistryLookupReturnsCorrectEntryIfFound() { |
There was a problem hiding this comment.
Really nice structured test which is easy to understand and the current grouping approach is perfectly readable.
Anyways maybe it is worth considering splitting this test in three separate Tests, in case of a failure the test name clarifies which registry search doesn't work immediatly
4 participants
This PR fixes #1869
Implemented changes:
DisplayVersion,DisplayIcon,InstallLocationandUninstallStringin common registry paths for an application.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)with
internal