fix(tests): issue #1200, added testing compatibility for jdk9+

[devsomelife]

This solves the "await/Continuations error" when trying to run tests in if you're using jdk9+

[tomparle]

Hi SomelifeDev,

I confirm this solves the Cannot use await/continuations when not all application classes on the callstack are properly enhanced. The following class is not enhanced: jdk.internal.reflect.NativeMethodAccessorImpl error when using jdk9.
I also confirmed that it worked with jdk10.

There is an additional warning that I'm not qualified enough to say if it's important or not :
WARNING: Illegal reflective access by javassist.util.proxy.SecurityActions (file:.../play-1.x/framework/lib/javassist-3.21.0-GA.jar) to method java.lang.ClassLoader.defineClass(java.lang.String,byte[],int,int,java.security.ProtectionDomain)

So, thanks a lot ! I am looking forward this fix to be merged since it would finally allows us to use Play 1.x with more recent versions of JDK.

[tazmaniax]

I would say that I'm equally unqualified to resolve :) but looking at the javassist release notes, v3.22 introduced support for JDK 9 and the latest GA release is v3.23.1. If it's possible to upgrade javassist then that's probably worth doing. If that's not possible or doesn't resolve it then it's possible to open up the required modules for javaasist which can be found in this migration guide - see slide 16 and 31.

[tomparle]

Hi Chris,

thank you for the quick reply !
Indeed, upgrading to the latest version of javassist did the trick !
I created the corresponding pull request which only required a small change : #1247

Please tell me if the pull request is ok or if I should have done something differently, I'm not that much used to contribute, yet !
Tom

[asolntsev]

@SomelifeDev is there a unit-test for class Controller? It would be a good practice to add one more test-case to that unit-test.

[tazmaniax]

Connecting this to #1200

[devsomelife]

Hey, glad it works and great one for the update on the javassist: quite obvious but nobody bothered apparently, until now.

@asolntsev hmm, not sure to be honest. This is just to fix the inability to use the built-in play test system on jdk 9+

[tomparle]

@SomelifeDev it's not that we did not bother, but personally I had no idea of how to fix this initially ! So thank you for having unblocked the situation.
Regarding to the corresponding test case, while it seems that there is no test class for Controller specifically, among all tests in error, there is the test ContinuationTest that explicitely failed with jdk9+, but passed with SomelifeDev's fix.
So maybe I guess we may not need to add another test specifically, what do you think ?

[asolntsev]

@tomparle @SomelifeDev Regarding a unit-test. I mean, in normal situation I would

1. extract className.startsWith("sun.") || className.startsWith("play.") to a separate method in Controller class. Say,

static boolean shouldNotBeEnhanced(String className) {
  return className.startsWith("sun.") || className.startsWith("play.");
}

2. I would create a unit-test ControllerTest.java with couple of asserts:

@Test
public void jdkAndPlayClassesShouldNeverBeenEnhanced() {
  assertTrue(shouldNotBeEnhanced("sun.blah"));
  assertTrue(shouldNotBeEnhanced("play.foo"));
  assertFalse(shouldNotBeEnhanced("com.bar"));
}

But in this case I guess we can omit this step because there is so much code that is not covered by unit-tests at all.

[tomparle]

@asolntsev thanks for your test suggestion, sounds right to me, especially because it enlightens the "ignore for enhancement" mechanism, which is otherwise kind of hidden in Play internals.
@SomelifeDev what do you think ? it should be quick to add this test.
Otherwise I will add it next week (not sure if I can contribute to the pull request of another user ?)

[devsomelife]

@asolntsev and @tomparle
Done (after a couple massive monday morning brain farts).
Was a good idea and even if lot of tests are missing, it's the occasion to start on the ControllerTest class anyway. Plus allowed me to finally take the time to properly set things up on my side...

[asolntsev]

@SomelifeDev Great work, thank you!
I have two more wishes:

1. Method shouldBeCheckedForEnhancement could be simplified:

static boolean shouldBeCheckedForEnhancement(String className) {
  return className.startsWith("jdk.") || className.startsWith("sun.") || className.startsWith("play.");
}

2. Could you squach your N commits into one commit? It would simplify git history tree.

[devsomelife]

@asolntsev done and done.

[asolntsev]

@SomelifeDev Thank you! Merged.

[tomparle]

@SomelifeDev thank you for the fix and having taken into account @asolntsev 's suggestions !

[tazmaniax]

Sorry for being late to the party but this comment feels counter intuitive. It states that if the class name package is jdk, sun or play then it should NOT be checked for enhancement. As the method returns true if any of those packages match then shouldn't the method be shouldNotBeCheckedForEnhancement() ? Or am I having a brain fart here :)

[asolntsev]

Yes, good point. I already fixed it in master branch.

[devsomelife]

@tazmaniax
You're 100% correct