Non-deterministic order in the message produced by CommandLine.MutuallyExclusiveArgsException

[kaiyaok2]

Description of the issue: In BuildTest, WarTest and JarTest of the cli module, the tests verify incompatible credential options passed to cli by checking the message of a raised MutuallyExclusiveArgsException. However, Picocli command line uses a Set to keep track of options, hence the exception message String does not ensure the order of the conflict credential options to be appeared. Notice that the tests pass right now, but may be flaky when being run in a different JVM, or when Java upgrades in the future. (E.g. HashSet default iteration order changed between Java 7 and 8, breaking lots of unit tests.) Our tool NonDex (https://github.com/TestingResearchIllinois/NonDex) permutes HashSet iterations just as between Java 7 and 8, and was able to report the above issue.

Log of Test Failure in a different JVM:

com.google.cloud.tools.jib.cli.WarTest > testParse_incompatibleCredentialOptions(--username=x, --password=x, --to-username=x, --to-password=x) [6] FAILED
    value of                       : throwable.getMessage()
    expected to contain a match for: ^Error: (--(from-|to-)?credential-helper|\[--username)
    but was                        : Error: [--password[=<password>] --username=<username>] and [[--to-credential-helper=<credential-helper> | [--to-username=<username> --to-password[=<password>]]] [--from-credential-helper=<credential-helper> | [--from-username=<username> --from-password[=<password>]]]] are mutually exclusive (specify only one)
        at com.google.cloud.tools.jib.cli.WarTest.testParse_incompatibleCredentialOptions(WarTest.java:435)

Notice that --username may appear after --password in the message, causing the regex matching routine to break.

Steps to Reproduce:

1. In build.gradle of the root directory, add id 'edu.illinois.nondex' version "2.1.1" as a plugin.
2. Run ./gradlew jib-cli:nondexTest --tests=WarTest in the root directory.

Proposed Changes: Modify the regular expression matching pattern (e.g. line 437 in JarTest.java) to account for possible order permutation.

I can open a small PR to fix this if this proposal seems reasonable!

[blakeli0]

@mpeddada1 Do you think this proposal is reasonable?

[mpeddada1]

Thanks for the detailed investigation @kaiyaok2! We're looking for opportunities to make our tests less brittle/flaky, please feel free to open a PR. Perhaps we can start with one test and then expand if we think that the change makes sense? Additionally, for the issue itself, is this something that can brought up in https://github.com/remkop/picocli?

[kaiyaok2]

Thanks for the detailed investigation @kaiyaok2! We're looking for opportunities to make our tests less brittle/flaky, please feel free to open a PR. Perhaps we can start with one test and then expand if we think that the change makes sense? Additionally, for the issue itself, is this something that can brought up in https://github.com/remkop/picocli?

Thanks for the review! I think it makes sense for Picocli cli to not specify determined order of credential options (as its functionality will not need to rely on input order, and unit tests in jib-cli shall not either.

I proposed a simple fix in one of the tests in https://github.com/GoogleContainerTools/jib/pull/4093.

[mpeddada1]

Thank you so much for your contribution @kaiyaok2! Closing this issue now that https://github.com/GoogleContainerTools/jib/pull/4103 and https://github.com/GoogleContainerTools/jib/pull/4093 are merged.