Introduce Arquillian adapter #2481
Conversation
mkouba
commented
May 17, 2019
mkouba
commented
- resolves Create Arquillian adapter #206
- make use of BootstrapClassLoaderFactory
- register HTTPContext to make @ArquillianResource URI injectable
mkouba
requested review from
stuartwdouglas,
geoand,
gsmet,
aloubyansky and
kenfinnigan
|
@bartoszmajsak Could you pls look at the arquillian parts? |
test-framework/arquillian/src/main/java/io/quarkus/arquillian/QuarkusDeployableContainer.java
Show resolved
Hide resolved
extensions/arc/runtime/src/main/java/io/quarkus/arc/runtime/ConfigDeploymentTemplate.java
Show resolved
Hide resolved
test-framework/arquillian/src/main/java/io/quarkus/arquillian/QuarkusDeployableContainer.java
Outdated
Show resolved
Hide resolved
Sure, will do this today. |
There was a problem hiding this comment.
Thanks for this work @mkouba. I see that we have just two smoke tests here. Is TCK the place with more sophisticated tests and that's where this adapter is going to be used?
I shared a few points to discuss.
test-framework/arquillian/src/main/java/io/quarkus/arquillian/QuarkusDeployableContainer.java
Show resolved
Hide resolved
|
|
||
| List<Consumer<BuildChainBuilder>> customizers = new ArrayList<>(); | ||
| try { | ||
| // Test class is a bean |
There was a problem hiding this comment.
Maybe extract to a method for better readability?
| try { | ||
| testClassesLocation = PathTestHelper.getTestClassesLocation(testJavaClass); | ||
| } catch (Exception e) { | ||
| // TCK tests are usually located in a dependency jar |
There was a problem hiding this comment.
Is it worth logging a warning here? Are we sure every exception can be handled by falling back to target/test-classes?
test-framework/arquillian/src/main/java/io/quarkus/arquillian/QuarkusDeployableContainer.java
Outdated
Show resolved
Hide resolved
test-framework/arquillian/src/main/java/io/quarkus/arquillian/QuarkusProtocol.java
Outdated
Show resolved
Hide resolved
|
I'm not terribly familiar with Arquillian, is there anything in particular that you would like me to review @mkouba? |
| @@ -0,0 +1,6 @@ | |||
| package io.quarkus.tck.config; | |||
|
|
|||
| // We need this class so that target/test-classes is added to the class path? | |||
There was a problem hiding this comment.
What breaks if target/test-classes isn't on the classpath?
There was a problem hiding this comment.
Well, it seems that surefire does not create a test classloader and I think we need this because of how framework classes are loaded. But I can try it again since I changed the code to use the BootstrapClassLoaderFactory...
There was a problem hiding this comment.
It fails without the dummy class. Maybe @stuartwdouglas or @aloubyansky would know why?
- resolves quarkusio#206 - make use of BootstrapClassLoaderFactory - register HTTPContext to make @ArquillianResource URI injectable - add TCK parent pom and skipNexusStagingDeployMojo - close the app CL after the test - add profile to run tcks
|
I've tried to address most of the comments (thanks all reviewers!) and I think that this PR is currently in a "ready-to-merge" state. Note that this Arquillian adapter is NOT going to be promoted as an official way of testing quarkus apps - it's purpose is to run MP TCKs (at least for now). In other words, I believe it's good enough for this purpose and of course we can always improve it later. PS. I feel like I have nothing more to say about this subject. I'd be more than happy if anyone can "take over the baton" and for example try configure other TCKs. |
|
@gsmet I think all your comments have been resolved. Can you re-review. |
