Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Upgrade to JUnit 4.12 #23877

Merged
merged 2 commits into from
May 2, 2017
Merged

Upgrade to JUnit 4.12 #23877

merged 2 commits into from
May 2, 2017

Conversation

dadoonet
Copy link
Member

@dadoonet dadoonet commented Apr 3, 2017 •
edited
Loading

@dadoonet
[WIP] Upgrade to JUnit 4.12
6046ae0 
By curiosity, I was just trying today to upgrade JUnit to 4.12 but this is not straight forward as now the SM is complaining.

```sh
gradle :test:framework:test -Dtests.jvm.argline="-Djava.security.debug=access,failure"
```

```
java.lang.Exception: Stack trace
  2>    at java.lang.Thread.dumpStack(Thread.java:1333)
  2>    at java.security.AccessControlContext.checkPermission(AccessControlContext.java:462)
  2>    at java.security.AccessController.checkPermission(AccessController.java:884)
  2>    at java.lang.SecurityManager.checkPermission(SecurityManager.java:549)
  2>    at java.lang.Class.checkMemberAccess(Class.java:2348)
  2>    at java.lang.Class.getDeclaredMethods(Class.java:1974)
  2>    at org.junit.internal.MethodSorter.getDeclaredMethods(MethodSorter.java:54)
  2>    at org.junit.runners.model.TestClass.scanAnnotatedMembers(TestClass.java:65)
  2>    at org.junit.runners.model.TestClass.<init>(TestClass.java:57)
  2>    at com.carrotsearch.randomizedtesting.RandomizedRunner$12.run(RandomizedRunner.java:1053)
  2>    at com.carrotsearch.randomizedtesting.RandomizedRunner$12.run(RandomizedRunner.java:1050)
  2>    at java.security.AccessController.doPrivileged(Native Method)
  2>    at com.carrotsearch.randomizedtesting.RandomizedRunner.getAnnotatedFieldValues(RandomizedRunner.java:1050)
  2>    at com.carrotsearch.randomizedtesting.RandomizedRunner.withClassRules(RandomizedRunner.java:885)
  2>    at com.carrotsearch.randomizedtesting.RandomizedRunner.runSuite(RandomizedRunner.java:674)
  2>    at com.carrotsearch.randomizedtesting.RandomizedRunner.access$200(RandomizedRunner.java:140)
  2>    at com.carrotsearch.randomizedtesting.RandomizedRunner$2.run(RandomizedRunner.java:598)
```

Just opening this PR for now as a WIP.
May be something super easy to fix...
@dadoonet dadoonet added the >test Issues or PRs that are addressing/adding tests label Apr 3, 2017
@dadoonet dadoonet self-assigned this Apr 3, 2017
@dadoonet dadoonet requested a review from jasontedor April 3, 2017 14:48
jasontedor
jasontedor requested changes Apr 3, 2017
View reviewed changes
Copy link
Member

@jasontedor jasontedor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You have to grant java.lang.RuntimePermission "accessDeclaredMembers" to JUnit 4.12 in test-framework.policy (replace what is already granted to JUnit 4.11). Also, you'll need to check if the junit4-ant dependency is updated and adjust accordingly.

@dadoonet
Copy link
Member Author

dadoonet commented Apr 3, 2017

Thanks @jasontedor

Something looks strange to me about junit4-ant. In master branch, we upgraded already randomized testing fw to 2.5.0 but I think we forgot to upgrade junit4-ant policy as I can see:

grant codeBase "${codebase.junit4-ant-2.3.2.jar}" {
  // needed for stream redirection
  permission java.lang.RuntimePermission "setIO";
};

Does it mean that we actually don't need anymore this policy? WDYT?

@dadoonet dadoonet changed the title [WIP] Upgrade to JUnit 4.12 Upgrade to JUnit 4.12 Apr 3, 2017
@dadoonet
Copy link
Member Author

dadoonet commented Apr 3, 2017 •
edited
Loading

I ran gradle test and it passed in elasticsearch master branch.

@rjernst
Copy link
Member

rjernst commented Apr 3, 2017

In master branch, we upgraded already randomized testing fw to 2.5.0 but I think we forgot to upgrade junit4-ant policy
Does it mean that we actually don't need anymore this policy?

That permission is still listed in the example security policy for RR:
https://github.com/randomizedtesting/randomizedtesting/blob/c3b588549ede1c3acf8c35f2b6dd66046cdda309/examples/security-manager/security.policy

@jasontedor
Copy link
Member

jasontedor commented Apr 3, 2017 •
edited
Loading

The java.lang.RuntimePermission "setIO" permission is indeed needed, randomized runner tries to set stdout and stderr for the slaves. Please return that permission with the correct version, otherwise we are at risk of losing valuable output.

@jasontedor
Copy link
Member

@rjernst and I looked at this more closely. The permission would be needed by the JVM that forks the slave JVMs. For us, this JVM does not run with a security manager, only the slave JVMs do in the common case. So, we in fact do not need this permission and it can remain removed.

jasontedor
jasontedor approved these changes Apr 3, 2017
View reviewed changes
Copy link
Member

@jasontedor jasontedor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

@dadoonet
Copy link
Member Author

dadoonet commented Apr 4, 2017

Thanks @jasontedor and @rjernst.

Do you think this should also go to 5.x branch?

@jasontedor
Copy link
Member

Do you think this should also go to 5.x branch?

Yes.

@dadoonet dadoonet merged commit 186b401 into elastic:master May 2, 2017
@dadoonet dadoonet deleted the pr/update-junit branch May 2, 2017 08:03
@dadoonet dadoonet added the v5.5.0 label May 2, 2017
dadoonet added a commit that referenced this pull request May 2, 2017
@dadoonet
Upgrade to JUnit 4.12 (#23877)
fb21caf 
* Upgrade to JUnit 4.12
* Add permission to junit 4.12 and remove junit4-ant specific permission

Backport of #23877 in 5.x branch
@dadoonet
Copy link
Member Author

dadoonet commented May 2, 2017

Pushed in master/5.x (5.5.0). Thanks!

jasontedor added a commit to jasontedor/elasticsearch that referenced this pull request May 2, 2017
@jasontedor
Merge branch 'master' into block-global-checkpoint-advancement
ee93332 
* master: (27 commits)
  Check index sorting with no replica since we cannot ensure that the replica index is ready when forceMerge is called. Closes elastic#24416
  Docs: correct indentation on callout
  Build that java api docs from a test (elastic#24354)
  Move RemoteClusterService into TransportService (elastic#24424)
  Fix license header in WildflyIT.java
  Try not to lose stacktraces (elastic#24426)
  [DOCS] Update XPack Reference URL for 5.4 (elastic#24425)
  Painless: Add tests to check for existence and correct detection of the special Java 9 optimizations: Indified String concat and MethodHandles#ArrayLengthHelper() (elastic#24405)
  Extract a common base class to allow services to listen to remote cluster config updates (elastic#24367)
  Adds check to snapshot repository incompatible-snapshots blob to delete a pre-existing one before attempting to overwrite it.
  Added docs for batched_reduce_size
  Fixes checkstyle errors
  Allow scripted metric agg to access `_score` (elastic#24295)
  [Test] Add unit tests for HDR/TDigest PercentilesAggregators (elastic#24245)
  Fix FieldCaps documentation
  Upgrade to JUnit 4.12 (elastic#23877)
  Set available processors for Netty
  Painless: Fix method references to ctor with the new LambdaBootstrap and cleanup code (elastic#24406)
  Doc test: use propery regex for file size
  [DOCS] Tweak doc test to sync_flush
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jasontedor jasontedor jasontedor approved these changes

Assignees

@dadoonet dadoonet

Labels
>test Issues or PRs that are addressing/adding tests v5.5.0 v6.0.0-alpha1
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@dadoonet @rjernst @jasontedor