On Mon, 14 Feb 2022 05:27:39 GMT, Jaikiran Pai <j...@openjdk.org> wrote:
> Can I please get a review for this change which proposes to fix the issue > noted in https://bugs.openjdk.java.net/browse/JDK-8281634? > > The commit introduces the missing `err.invalid.filters` key in the jdeps > resource bundle. I have run a simple check to make sure this resource bundle > doesn't miss any other `err.` keys. From a simple search, following are the > unique `err.` keys used in the code of jdeps (under > `src/jdk.jdeps/share/classes/com/sun/tools/jdeps` directory): > > err.exception.message > err.invalid.options > err.multirelease.version.associated > err.missing.arg > err.multirelease.jar.malformed > err.option.already.specified > err.missing.dependences > err.module.not.found > err.invalid.path > err.genmoduleinfo.not.jarfile > err.invalid.arg.for.option > err.multirelease.option.notfound > err.filter.not.specified > err.unknown.option > err.command.set > err.invalid.filters > err.genmoduleinfo.unnamed.package > err.option.after.class > > Apart from the `err.invalid.filters` key which this PR is fixing, none of the > other keys are missing from the resource bundle. I haven't updated the > resource bundles for Japanese and Chinese languages because I don't know > their equivalent values and looking at the commit history of those files, it > appears that those changes are done as separate tasks (should a JBS issue be > raised for this?) > > The existing `test/langtools/tools/jdeps/Options.java` jtreg test has been > updated to reproduce the issue and verify this fix. > > An important detail about the update to the test is that while working on the > update to this test, I realized that the current implementation in the test > isn't fully functional. As a result, this test is currently, incorrectly > considered as passing. Specifically, the test was missing a `assertTrue` call > against the ouput/error content generated by the run of the `jdeps` tool. > This PR adds that assertion. > Once that assertion is added, it started showing up 3 genuine failures. These > failures are within that test code: > - The test uses `-jdkinternal` instead of `-jdkinternals`. This appears to be > a typo when this section in the test was added. The commit history of the > source of jdeps tool shows that this option was always `-jdkinternals`. This > PR fixes that part in the test. > - The test expects an error message "-R cannot be used with --inverse option" > when `-R` and `--inverse` are used together. However, at some point the > source of jdeps tool was changed (as part of > https://bugs.openjdk.java.net/browse/JDK-8213909) to return a different error > message. That changes appears to have missed changing this test case error > message and since this test has been falsely passing, it never got caught. > This PR now fixes that issue by expecting the correct error message. > - The test was expecting an error message "--list-deps and > --list-reduced-deps options are specified" when "--list-deps" was used along > with "--summary". This appears to be a copy/paste error in the test case and > wasn't caught because the test was falsely passing. This too has been fixed > in this PR. > > With the fixes in this test, the test now reproduces the original issue and > verifies the fix. I realize that this PR has changes in the test that aren't > directly related to the issue raised in > https://bugs.openjdk.java.net/browse/JDK-8281634, but those changes are > necessary to get the test functional. However if a separate JBS issue needs > to be opened to track those changes, please do let me know and I'll address > these test changes in a separate PR. Thank you for your review, Daniel. > Maybe wait one day or two before integrating to give Mandy a chance to chime > in. Certainly. ------------- PR: https://git.openjdk.java.net/jdk/pull/7455
