Explicitly set illegal-access to deny for tests #72588
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Since Java 16, the default value for illegal-access is deny. This means the latest release of Elasticsearch, and all current integration tests, run with deny (since we don't explicitly set it in jvm options). Yet tests run with illegal-access=warn, for legacy reasons. elastic#71908 proposed to remove the setting from test jvms, but concerns were raised there about whether this would cause some test failures. This commit explicitly sets tests to deny. This has the added benefit that any failures will be caught even when running tests with older jvms.
rjernst
added
>test
|
Pinging @elastic/es-delivery (Team:Delivery) |
|
@mark-vieira This is ready for review. It at least gets our tests closer to what production is using (since illegal-access defaults to deny in Java 16). I will followup trying to make mockito modular so that we can not accidentally allow production code to cross the module boundaries of the JDK as necessary in tests for mocking. |
mark-vieira
approved these changes
Jun 1, 2021
There was a problem hiding this comment.
Looks like you still have one mole left to whack but otherwise LGTM.
Caused by: java.lang.IllegalAccessException: class org.apache.hadoop.security.authentication.util.KerberosUtil cannot access class sun.security.krb5.Config (in module java.security.jgss) because module java.security.jgss does not export sun.security.krb5 to unnamed module @77cd01d8
|
Looks related to #68075. Of course the failure doesn't reproduce...will investigate. |
|
@elasticmachine update branch |
rjernst
added a commit
to rjernst/elasticsearch
that referenced
this pull request
Jun 2, 2021
Since Java 16, the default value for illegal-access is deny. This means the latest release of Elasticsearch, and all current integration tests, run with deny (since we don't explicitly set it in jvm options). Yet tests run with illegal-access=warn, for legacy reasons. elastic#71908 proposed to remove the setting from test jvms, but concerns were raised there about whether this would cause some test failures. This commit explicitly sets tests to deny. This has the added benefit that any failures will be caught even when running tests with older jvms.
rjernst
added a commit
that referenced
this pull request
Jun 3, 2021
Since Java 16, the default value for illegal-access is deny. This means the latest release of Elasticsearch, and all current integration tests, run with deny (since we don't explicitly set it in jvm options). Yet tests run with illegal-access=warn, for legacy reasons. #71908 proposed to remove the setting from test jvms, but concerns were raised there about whether this would cause some test failures. This commit explicitly sets tests to deny. This has the added benefit that any failures will be caught even when running tests with older jvms.
rjernst
added a commit
to rjernst/elasticsearch
that referenced
this pull request
Sep 23, 2021
Since Java 16, the default value for illegal-access is deny. This means the latest release of Elasticsearch, and all current integration tests, run with deny (since we don't explicitly set it in jvm options). Yet tests run with illegal-access=warn, for legacy reasons. elastic#71908 proposed to remove the setting from test jvms, but concerns were raised there about whether this would cause some test failures. This commit explicitly sets tests to deny. This has the added benefit that any failures will be caught even when running tests with older jvms.
elasticsearchmachine
pushed a commit
that referenced
this pull request
Sep 23, 2021
Since Java 16, the default value for illegal-access is deny. This means the latest release of Elasticsearch, and all current integration tests, run with deny (since we don't explicitly set it in jvm options). Yet tests run with illegal-access=warn, for legacy reasons. #71908 proposed to remove the setting from test jvms, but concerns were raised there about whether this would cause some test failures. This commit explicitly sets tests to deny. This has the added benefit that any failures will be caught even when running tests with older jvms.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Since Java 16, the default value for illegal-access is deny. This means
the latest release of Elasticsearch, and all current integration tests,
run with deny (since we don't explicitly set it in jvm options). Yet
tests run with illegal-access=warn, for legacy reasons. #71908
proposed to remove the setting from test jvms, but concerns were raised
there about whether this would cause some test failures.
This commit explicitly sets tests to deny. This has the added benefit
that any failures will be caught even when running tests with older
jvms.