[GitHub] [tomee] rzo1 opened a new pull request #767: TOMEE-2977 - Provide a ApplicationComposer JUnit 5 Extension

classic Classic list List threaded Threaded
43 messages Options
123
Reply | Threaded
Open this post in threaded view
|

[GitHub] [tomee] rzo1 opened a new pull request #767: TOMEE-2977 - Provide a ApplicationComposer JUnit 5 Extension

GitBox

rzo1 opened a new pull request #767:
URL: https://github.com/apache/tomee/pull/767


   # What does this PR do?
   
   - Adds a JUnit 5 extension, which internally uses `ApplicationComposers` for (pure) JUnit 5 testing.
   - Adds some unit tests (based on existing `ApplicationComposer` tests) written in JUnit 5
   - Provides an additional JUnit 5 example (based on `application-composer`)
   - Upgrades Maven surefire to `2.22.1` (  `>=2.22.0` is required to run both variants of unit tests without workarounds out of the box)
   
   # References
   
   - https://issues.apache.org/jira/projects/TOMEE/issues/TOMEE-2977


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]


Reply | Threaded
Open this post in threaded view
|

[GitHub] [tomee] rmannibucau commented on a change in pull request #767: TOMEE-2977 - Provide a ApplicationComposer JUnit 5 Extension

GitBox

rmannibucau commented on a change in pull request #767:
URL: https://github.com/apache/tomee/pull/767#discussion_r590292456



##########
File path: container/openejb-core/src/main/java/org/apache/openejb/junit/jupiter/ApplicationComposerExtension.java
##########
@@ -0,0 +1,47 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.openejb.junit.jupiter;
+
+import org.apache.openejb.OpenEJBRuntimeException;
+import org.apache.openejb.testing.ApplicationComposers;
+import org.junit.jupiter.api.extension.*;
+
+public class ApplicationComposerExtension implements BeforeEachCallback, AfterEachCallback {

Review comment:
       A few open questions:
   
   1. isnt it beforeall/afterall callbacks? (way faster)
   2. shouldn't "delegate" be stored in ExtensionContext to ensure it is well scoped and not limited to this particular threading+scope which is one of the multiple junit5 cases
   3. What about other test instances you can get from the context? should they contribute to the app model?
   4. Should it get a "Single"/mono extension (started once per jvm as tomee embedded), it is generally accurate for apps written this way
   5. (very minor) I wouldn't mention extension in the annotation "ApplicatioComposerExtension" name (maybe `@WithApplicationComposer` or alike)
   
   hope it makes sense




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]


Reply | Threaded
Open this post in threaded view
|

[GitHub] [tomee] rzo1 commented on a change in pull request #767: TOMEE-2977 - Provide a ApplicationComposer JUnit 5 Extension

GitBox
In reply to this post by GitBox

rzo1 commented on a change in pull request #767:
URL: https://github.com/apache/tomee/pull/767#discussion_r590340268



##########
File path: container/openejb-core/src/main/java/org/apache/openejb/junit/jupiter/ApplicationComposerExtension.java
##########
@@ -0,0 +1,47 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.openejb.junit.jupiter;
+
+import org.apache.openejb.OpenEJBRuntimeException;
+import org.apache.openejb.testing.ApplicationComposers;
+import org.junit.jupiter.api.extension.*;
+
+public class ApplicationComposerExtension implements BeforeEachCallback, AfterEachCallback {

Review comment:
       Wow. Super fast :+1:
   
   1. I thought about it: Doesn't `ApplicationComposers` require a concrete test instance to inject values and stuff in `delegate.before(...)`?` The required test instance is - afaik - not available when `beforeAll(...)` is called. However, the creation of the `delegate` might be conducted in a `beforeAll(...) call.
   2. This is a good idea. I think, it can be done via `context.getRoot().getStore(...)`  the `ExtensionContext`.
   3. Honestly, I did not thought about this use-case. We may get additional test instances via `context.getRequiredTestInstances().getAllInstances()`. Wdyt?
   4. Should be very similar to `SingleApplicationComposerRunner` - I will take a look at it.
   5. Good idea. Will change it to `RunWith...` to be inline with the namings in `openejb-junit5-backward`.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]


Reply | Threaded
Open this post in threaded view
|

[GitHub] [tomee] rzo1 commented on a change in pull request #767: TOMEE-2977 - Provide a ApplicationComposer JUnit 5 Extension

GitBox
In reply to this post by GitBox

rzo1 commented on a change in pull request #767:
URL: https://github.com/apache/tomee/pull/767#discussion_r590340268



##########
File path: container/openejb-core/src/main/java/org/apache/openejb/junit/jupiter/ApplicationComposerExtension.java
##########
@@ -0,0 +1,47 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.openejb.junit.jupiter;
+
+import org.apache.openejb.OpenEJBRuntimeException;
+import org.apache.openejb.testing.ApplicationComposers;
+import org.junit.jupiter.api.extension.*;
+
+public class ApplicationComposerExtension implements BeforeEachCallback, AfterEachCallback {

Review comment:
       Wow. Super fast :+1:
   
   1. I thought about it: Doesn't `ApplicationComposers` require a concrete test instance to inject values and stuff in `delegate.before(...)`? The required test instance is - afaik - not available when `beforeAll(...)` is called. However, the creation of the `delegate` might be conducted in a `beforeAll(...) call.
   2. This is a good idea. I think, it can be done via `context.getRoot().getStore(...)`  the `ExtensionContext`.
   3. Honestly, I did not thought about this use-case. We may get additional test instances via `context.getRequiredTestInstances().getAllInstances()`. Wdyt?
   4. Should be very similar to `SingleApplicationComposerRunner` - I will take a look at it.
   5. Good idea. Will change it to `RunWith...` to be inline with the namings in `openejb-junit5-backward`.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]


Reply | Threaded
Open this post in threaded view
|

[GitHub] [tomee] rmannibucau commented on a change in pull request #767: TOMEE-2977 - Provide a ApplicationComposer JUnit 5 Extension

GitBox
In reply to this post by GitBox

rmannibucau commented on a change in pull request #767:
URL: https://github.com/apache/tomee/pull/767#discussion_r590380714



##########
File path: container/openejb-core/src/main/java/org/apache/openejb/junit/jupiter/ApplicationComposerExtension.java
##########
@@ -0,0 +1,47 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.openejb.junit.jupiter;
+
+import org.apache.openejb.OpenEJBRuntimeException;
+import org.apache.openejb.testing.ApplicationComposers;
+import org.junit.jupiter.api.extension.*;
+
+public class ApplicationComposerExtension implements BeforeEachCallback, AfterEachCallback {

Review comment:
       1. to inject anything yes but you don't always have to inject. That said point was the opposite: what if you split your test definition in multiple classes? What if you use a dynamic test? I suspect it can be worth a `@ApplicationComposer(mode = [PER_JVM|PER_ALL|PER_EACH])` - note that per_all/per_each are not great but per_class/per_method are not accurate with junit5 so feel free to find a better name, just sharing the concept I have in mind.
   2. no getRoot(), ensure to align the scope on the actual one: ` context.getStore(NAMESPACE)` with `private static final ExtensionContext.Namespace NAMESPACE = ExtensionContext.Namespace.create(TheExtension.class.getName());` probably
   3. right but do not used required accessor but optional one ;)




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]


Reply | Threaded
Open this post in threaded view
|

[GitHub] [tomee] rzo1 commented on a change in pull request #767: TOMEE-2977 - Provide a ApplicationComposer JUnit 5 Extension

GitBox
In reply to this post by GitBox

rzo1 commented on a change in pull request #767:
URL: https://github.com/apache/tomee/pull/767#discussion_r590433216



##########
File path: container/openejb-core/src/main/java/org/apache/openejb/junit/jupiter/ApplicationComposerExtension.java
##########
@@ -0,0 +1,47 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.openejb.junit.jupiter;
+
+import org.apache.openejb.OpenEJBRuntimeException;
+import org.apache.openejb.testing.ApplicationComposers;
+import org.junit.jupiter.api.extension.*;
+
+public class ApplicationComposerExtension implements BeforeEachCallback, AfterEachCallback {

Review comment:
       Thx. I updated things related to 2. and 3.
   
   I have to think about 1. (because I think, that I didnt get it) and read / study the documentation related to `DynamicTests` in JUnit 5. Do you have an example of "splitting test definitions into multiple classes"?
   
   Might get you wrong, but did you refer to  `Test Instance Lifecycles` when talking about `@ApplicationComposer(mode = [PER_JVM|PER_ALL|PER_EACH])`  ?
   
   So we get smth like this, if the mode is specified?
   
   - PER_JVM --> `SingleApplicationComposerExtension` (like TomEE embedded?)
   - PER_ALL --> `@TestInstance(Lifecycle.PER_CLASS)` //  new test instance will be created once per test class.
   - PER_EACH --> `@TestInstance(Lifecycle.PER_METHOD)` // a new test instance will be created for each test method, test factory method, or test template method (default in JUnit 4)
   
   




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]


Reply | Threaded
Open this post in threaded view
|

[GitHub] [tomee] rmannibucau commented on a change in pull request #767: TOMEE-2977 - Provide a ApplicationComposer JUnit 5 Extension

GitBox
In reply to this post by GitBox

rmannibucau commented on a change in pull request #767:
URL: https://github.com/apache/tomee/pull/767#discussion_r590508449



##########
File path: container/openejb-core/src/main/java/org/apache/openejb/junit/jupiter/ApplicationComposerExtension.java
##########
@@ -0,0 +1,47 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.openejb.junit.jupiter;
+
+import org.apache.openejb.OpenEJBRuntimeException;
+import org.apache.openejb.testing.ApplicationComposers;
+import org.junit.jupiter.api.extension.*;
+
+public class ApplicationComposerExtension implements BeforeEachCallback, AfterEachCallback {

Review comment:
       The split of an app composer app in multiple class is actually what you use ;). The constructor of ApplicationComposers takes N objects, just pass all test classes if you want a single deployment for all tests and enable each test to deploy part of the app.
   
   About the lifecycle it is not 1-1 with test instance lifecycle but the same idea for the container lifecycle (but you can have one test instance per test and one container instance for all tests of a single class when you want to reset another extension data/field instances for example). We can align on the default if nothing is set (so add to the enum an AUTO mode ;)) but must be configurable.
   
   About SingleApplicationComposerExtension i was thinking to https://github.com/apache/tomee/blob/master/tomee/tomee-embedded/src/main/java/org/apache/tomee/embedded/junit/TomEEEmbeddedSingleRunner.java or the same of meecrowave, when the app is fully modelized and you don't test a lib you only use that cause the perf boost is really impressive compared to starting one container per test.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]


Reply | Threaded
Open this post in threaded view
|

[GitHub] [tomee] rzo1 commented on a change in pull request #767: TOMEE-2977 - Provide a ApplicationComposer JUnit 5 Extension

GitBox
In reply to this post by GitBox

rzo1 commented on a change in pull request #767:
URL: https://github.com/apache/tomee/pull/767#discussion_r590638509



##########
File path: container/openejb-core/src/main/java/org/apache/openejb/junit/jupiter/ApplicationComposerExtension.java
##########
@@ -0,0 +1,47 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.openejb.junit.jupiter;
+
+import org.apache.openejb.OpenEJBRuntimeException;
+import org.apache.openejb.testing.ApplicationComposers;
+import org.junit.jupiter.api.extension.*;
+
+public class ApplicationComposerExtension implements BeforeEachCallback, AfterEachCallback {

Review comment:
       Thanks for the details!
   
   > The split of an app composer app in multiple class is actually what you use ;). The constructor of ApplicationComposers takes N objects, just pass all test classes if you want a single deployment for all tests and enable each test to deploy part of the app.
   
   Afaik the extension context only provides the related test class. So it would be something like
   
   - Get inner classes from the test class
   - Create instances of these inner test classes
   - Supply the test class as well as the N instances of the inner test classes to  `ApplicationComposers` (like it is done in `AppComposerAsJUnitRuleWithReusableModulesTest` via JUnit 4 rules)  ?
   
   > About the lifecycle it is not 1-1 with test instance lifecycle but the same idea for the container lifecycle (but you can have one test instance per test and one container instance for all tests of a single class when you want to reset another extension data/field instances for example). We can align on the default if nothing is set (so add to the enum an AUTO mode ;)) but must be configurable.
   
   Thanks!  I think, I am now getting a better understanding of the idea. I also checked the JUnit 5 extension impl of meecrowave, which was really helpful.
   
   
   
   




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]


Reply | Threaded
Open this post in threaded view
|

[GitHub] [tomee] rzo1 commented on a change in pull request #767: TOMEE-2977 - Provide a ApplicationComposer JUnit 5 Extension

GitBox
In reply to this post by GitBox

rzo1 commented on a change in pull request #767:
URL: https://github.com/apache/tomee/pull/767#discussion_r591543473



##########
File path: container/openejb-core/src/main/java/org/apache/openejb/junit/jupiter/ApplicationComposerExtension.java
##########
@@ -0,0 +1,47 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.openejb.junit.jupiter;
+
+import org.apache.openejb.OpenEJBRuntimeException;
+import org.apache.openejb.testing.ApplicationComposers;
+import org.junit.jupiter.api.extension.*;
+
+public class ApplicationComposerExtension implements BeforeEachCallback, AfterEachCallback {

Review comment:
       I made some progress based on your feedback, but I think it is still heavily **WIP** ;)  It should now honor the test instance lifecycle. The `mode` thing is missing yet. I have to think about the impl for the proposed `mode` for controlling the container instances created during tests.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]


Reply | Threaded
Open this post in threaded view
|

[GitHub] [tomee] rzo1 commented on a change in pull request #767: TOMEE-2977 - Provide a ApplicationComposer JUnit 5 Extension

GitBox
In reply to this post by GitBox

rzo1 commented on a change in pull request #767:
URL: https://github.com/apache/tomee/pull/767#discussion_r592724306



##########
File path: container/openejb-core/src/main/java/org/apache/openejb/junit/jupiter/ApplicationComposerExtension.java
##########
@@ -0,0 +1,47 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.openejb.junit.jupiter;
+
+import org.apache.openejb.OpenEJBRuntimeException;
+import org.apache.openejb.testing.ApplicationComposers;
+import org.junit.jupiter.api.extension.*;
+
+public class ApplicationComposerExtension implements BeforeEachCallback, AfterEachCallback {

Review comment:
       The current impl relies on the test's life cycle to start a container or perform related injections.
   
   Atm the impl contains annotations and extensions for
   
   - `RunWithSingleApplicationComposer`
   - `RunWithApplicationComposer`
   
   as well as a
   
   - `TomEEEmbeddedExtension`
   
   From my understanding, the proposed `mode` is solely responsible to configure how (or when) the underlying container is started? I.e. one container to run all tests within a single test class (PER_ALL), one container for each test method of one test class (PER_EACH) and one container to run multiple test classes (PER_JVM), right?  Is this the same idea, which is currently conducted through different JUnit4 runners?
   
   From my understanding of the current JUnit implementation, the `ApplicationComposer` seems to be something similar to the PER_ALL and the `EmbeddedTomEERunner` / `SingleApplicationComposer` to the PER_JVM; I think PER_EACH (test method) isn't implemented in the JUnit 4 runners yet.
   
   Probably I am missing something but I think, I do not have the whole process or related use-cases in mind ;) - Maybe you , @rmannibucau , can add some more ideas / thoughts on the `mode` idea?




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]


Reply | Threaded
Open this post in threaded view
|

[GitHub] [tomee] rzo1 commented on a change in pull request #767: TOMEE-2977 - Provide a ApplicationComposer JUnit 5 Extension

GitBox
In reply to this post by GitBox

rzo1 commented on a change in pull request #767:
URL: https://github.com/apache/tomee/pull/767#discussion_r592724306



##########
File path: container/openejb-core/src/main/java/org/apache/openejb/junit/jupiter/ApplicationComposerExtension.java
##########
@@ -0,0 +1,47 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.openejb.junit.jupiter;
+
+import org.apache.openejb.OpenEJBRuntimeException;
+import org.apache.openejb.testing.ApplicationComposers;
+import org.junit.jupiter.api.extension.*;
+
+public class ApplicationComposerExtension implements BeforeEachCallback, AfterEachCallback {

Review comment:
       The current impl relies on the test's life cycle to start a container or perform related injections.
   
   Atm the impl contains annotations and extensions for
   
   - `RunWithSingleApplicationComposer`
   - `RunWithApplicationComposer`
   
   as well as a
   
   - `TomEEEmbeddedExtension`
   
   From my understanding, the proposed `mode` is solely responsible to configure how (or when) the underlying container is started? I.e. one container to run all tests within a single test class (PER_ALL), one container for each test method of one test class (PER_EACH) and one container to run multiple test classes (PER_JVM), right?  Is this the same idea, which is currently conducted through different JUnit4 runners?
   
   From my understanding of the current JUnit implementation, the `ApplicationComposer` seems to be something similar to the PER_ALL and the `EmbeddedTomEERunner` / `SingleApplicationComposer` to the PER_JVM; I think PER_EACH (test method) isn't implemented in the JUnit 4 runners yet.
   
   Probably I am missing something but I think, I do not have the whole process or related use-cases in mind ;) - Maybe you , @rmannibucau , can add some more ideas / thoughts on the `mode` idea?
   
   EDIT-1: I added some code to draft my idea / thoughts related to `modes`. Might be better to understand from the code point of view than from my words, if I got the idea correctly: https://github.com/rzo1/tomee/pull/4/files




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]


Reply | Threaded
Open this post in threaded view
|

[GitHub] [tomee] rzo1 commented on a change in pull request #767: TOMEE-2977 - Provide a ApplicationComposer JUnit 5 Extension

GitBox
In reply to this post by GitBox

rzo1 commented on a change in pull request #767:
URL: https://github.com/apache/tomee/pull/767#discussion_r592724306



##########
File path: container/openejb-core/src/main/java/org/apache/openejb/junit/jupiter/ApplicationComposerExtension.java
##########
@@ -0,0 +1,47 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.openejb.junit.jupiter;
+
+import org.apache.openejb.OpenEJBRuntimeException;
+import org.apache.openejb.testing.ApplicationComposers;
+import org.junit.jupiter.api.extension.*;
+
+public class ApplicationComposerExtension implements BeforeEachCallback, AfterEachCallback {

Review comment:
       The current impl relies on the test's life cycle to start a container or perform related injections.
   
   Atm the impl contains annotations and extensions for
   
   - `RunWithSingleApplicationComposer`
   - `RunWithApplicationComposer`
   
   as well as a
   
   - `TomEEEmbeddedExtension`
   
   From my understanding, the proposed `mode` is solely responsible to configure how (or when) the underlying container is started? I.e. one container to run all tests within a single test class (PER_ALL), one container for each test method of one test class (PER_EACH) and one container to run multiple test classes (PER_JVM), right?  Is this the same idea, which is currently conducted through different JUnit4 runners?
   
   From my understanding of the current JUnit implementation, the `ApplicationComposer` seems to be something similar to the PER_ALL and the `EmbeddedTomEERunner` / `SingleApplicationComposer` to the PER_JVM; I think PER_EACH (test method) isn't implemented in the JUnit 4 runners yet.
   
   Probably I am missing something but I think, I do not have the whole process or related use-cases in mind ;) - Maybe you , @rmannibucau , can add some more ideas / thoughts on the `mode` idea?
   
   EDIT-1: I added some code to draft my idea / thoughts related to `modes`. Might be better to understand from the code point of view than from my words, if I got the idea correctly, so I added the related commits here.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]


Reply | Threaded
Open this post in threaded view
|

[GitHub] [tomee] rmannibucau commented on a change in pull request #767: TOMEE-2977 - Provide a ApplicationComposer JUnit 5 Extension

GitBox
In reply to this post by GitBox

rmannibucau commented on a change in pull request #767:
URL: https://github.com/apache/tomee/pull/767#discussion_r595289263



##########
File path: container/openejb-core/src/main/java/org/apache/openejb/junit/jupiter/ApplicationComposerExtensionBase.java
##########
@@ -26,4 +27,36 @@ boolean isPerClass(final ExtensionContext context) {
                 .map(it -> it.equals(TestInstance.Lifecycle.PER_CLASS))
                 .orElse(false);
     }
+
+    boolean isPerEach(final ExtensionContext context) {
+        return checkMode(context, ExtensionMode.PER_EACH);
+    }
+
+    boolean isPerAll(final ExtensionContext context) {
+        return checkMode(context, ExtensionMode.PER_ALL);
+    }
+
+    boolean isPerJvm(final ExtensionContext context) {
+        return checkMode(context, ExtensionMode.PER_JVM);
+    }
+
+    boolean isPerDefault(final ExtensionContext context) {
+        return checkMode(context, ExtensionMode.AUTO);
+    }
+
+    boolean checkMode(final ExtensionContext context, ExtensionMode extensionMode ) {
+       return extensionMode == getModeFromAnnotation(context);
+    }
+
+    ExtensionMode getModeFromAnnotation(final ExtensionContext context) {
+        if (context.getTestClass().isPresent()) {
+
+            RunWithApplicationComposer a = context.getTestClass().get().getAnnotation(RunWithApplicationComposer.class);

Review comment:
       AnnotationUtils.findAnnotation ;) (will support junit5 stereotypes otherwise you loose it by pure reflection)




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]


Reply | Threaded
Open this post in threaded view
|

[GitHub] [tomee] rzo1 commented on a change in pull request #767: TOMEE-2977 - Provide a ApplicationComposer JUnit 5 Extension

GitBox
In reply to this post by GitBox

rzo1 commented on a change in pull request #767:
URL: https://github.com/apache/tomee/pull/767#discussion_r595302374



##########
File path: container/openejb-core/src/main/java/org/apache/openejb/junit/jupiter/ApplicationComposerExtensionBase.java
##########
@@ -26,4 +27,36 @@ boolean isPerClass(final ExtensionContext context) {
                 .map(it -> it.equals(TestInstance.Lifecycle.PER_CLASS))
                 .orElse(false);
     }
+
+    boolean isPerEach(final ExtensionContext context) {
+        return checkMode(context, ExtensionMode.PER_EACH);
+    }
+
+    boolean isPerAll(final ExtensionContext context) {
+        return checkMode(context, ExtensionMode.PER_ALL);
+    }
+
+    boolean isPerJvm(final ExtensionContext context) {
+        return checkMode(context, ExtensionMode.PER_JVM);
+    }
+
+    boolean isPerDefault(final ExtensionContext context) {
+        return checkMode(context, ExtensionMode.AUTO);
+    }
+
+    boolean checkMode(final ExtensionContext context, ExtensionMode extensionMode ) {
+       return extensionMode == getModeFromAnnotation(context);
+    }
+
+    ExtensionMode getModeFromAnnotation(final ExtensionContext context) {
+        if (context.getTestClass().isPresent()) {
+
+            RunWithApplicationComposer a = context.getTestClass().get().getAnnotation(RunWithApplicationComposer.class);

Review comment:
       Thx. Changed it.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]


Reply | Threaded
Open this post in threaded view
|

[GitHub] [tomee] rmannibucau commented on a change in pull request #767: TOMEE-2977 - Provide a ApplicationComposer JUnit 5 Extension

GitBox
In reply to this post by GitBox

rmannibucau commented on a change in pull request #767:
URL: https://github.com/apache/tomee/pull/767#discussion_r595343572



##########
File path: container/openejb-core/src/main/java/org/apache/openejb/junit/jupiter/ApplicationComposerExtension.java
##########
@@ -0,0 +1,155 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.openejb.junit.jupiter;
+
+import org.apache.openejb.OpenEJBRuntimeException;
+import org.apache.openejb.testing.ApplicationComposers;
+import org.apache.openejb.testing.SingleApplicationComposerBase;
+import org.junit.jupiter.api.extension.*;
+
+import java.util.Arrays;
+import java.util.List;
+import java.util.Objects;
+import java.util.Optional;
+
+public class ApplicationComposerExtension extends ApplicationComposerExtensionBase implements BeforeAllCallback, AfterAllCallback, BeforeEachCallback, AfterEachCallback {
+
+    private static final ExtensionContext.Namespace NAMESPACE = ExtensionContext.Namespace.create(ApplicationComposerExtension.class.getName());
+    private static final SingleApplicationComposerBase BASE = new SingleApplicationComposerBase();
+
+    @Override
+    public void beforeAll(ExtensionContext context) throws Exception {
+
+        if (isPerJvm(context)) {

Review comment:
       If you have per jvm and per test/class at the same time what happens? Options I see:
   
   1. test if per jvm is started and fail perall/pereach cases
   2. make it work (we did in meecrowave, it is mainly a matter of having a JVM singleton/lock to switch the tccl for the bootstrap time + a few singleton fixes)
   
   I'd start with 1 keeping 2 in mind (since it requires more work) so something like 'if !JVM && BASE.isStarted() => fail', isStarted is just matter of testing one of the atomic ref of the single instance.

##########
File path: container/openejb-core/src/main/java/org/apache/openejb/junit/jupiter/ApplicationComposerExtension.java
##########
@@ -0,0 +1,155 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.openejb.junit.jupiter;
+
+import org.apache.openejb.OpenEJBRuntimeException;
+import org.apache.openejb.testing.ApplicationComposers;
+import org.apache.openejb.testing.SingleApplicationComposerBase;
+import org.junit.jupiter.api.extension.*;
+
+import java.util.Arrays;
+import java.util.List;
+import java.util.Objects;
+import java.util.Optional;
+
+public class ApplicationComposerExtension extends ApplicationComposerExtensionBase implements BeforeAllCallback, AfterAllCallback, BeforeEachCallback, AfterEachCallback {
+
+    private static final ExtensionContext.Namespace NAMESPACE = ExtensionContext.Namespace.create(ApplicationComposerExtension.class.getName());
+    private static final SingleApplicationComposerBase BASE = new SingleApplicationComposerBase();
+
+    @Override
+    public void beforeAll(ExtensionContext context) throws Exception {
+
+        if (isPerJvm(context)) {
+            BASE.start(context.getTestClass().orElse(null));
+        } else if (isPerAll(context)) {
+            doStart(context);
+        } else if (isPerDefault(context)) {
+            if (isPerClass(context)) {
+                doStart(context);
+            }
+        }
+
+        if (isPerClass(context)) {
+            doInject(context);
+        }
+    }
+
+    @Override
+    public void afterAll(ExtensionContext context) throws Exception {
+        if (isPerJvm(context) || isPerAll(context)) {
+            doRelease(context);
+        } else if (isPerDefault(context) && isPerClass(context)) {
+            doRelease(context);
+        }
+    }
+
+
+    @Override
+    public void beforeEach(ExtensionContext context) {
+
+        if (isPerEach(context)) {
+            doStart(context);
+        } else if (isPerDefault(context) && !isPerClass(context)) {
+            doStart(context);
+        }
+
+        if (!isPerClass(context)) {
+            doInject(context);
+        }
+    }
+
+    @Override
+    public void afterEach(ExtensionContext context) throws Exception {
+        if (isPerEach(context)) {
+            doRelease(context);
+        } else if (isPerDefault(context) && !isPerClass(context)) {
+            doRelease(context);
+        }
+    }
+
+    private void doRelease(final ExtensionContext extensionContext) throws Exception {
+        if (isPerJvm(extensionContext)) {
+            //FIXME how to release / close after last test class execution?

Review comment:
       should be done with the shutdown hook
   this implementation does NOT work so it likely means it misses a test which should fail (like chaining 2 test classes and ensuring it uses the same container)

##########
File path: container/openejb-core/src/main/java/org/apache/openejb/junit/jupiter/ApplicationComposerExtension.java
##########
@@ -0,0 +1,155 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.openejb.junit.jupiter;
+
+import org.apache.openejb.OpenEJBRuntimeException;
+import org.apache.openejb.testing.ApplicationComposers;
+import org.apache.openejb.testing.SingleApplicationComposerBase;
+import org.junit.jupiter.api.extension.*;
+
+import java.util.Arrays;
+import java.util.List;
+import java.util.Objects;
+import java.util.Optional;
+
+public class ApplicationComposerExtension extends ApplicationComposerExtensionBase implements BeforeAllCallback, AfterAllCallback, BeforeEachCallback, AfterEachCallback {
+
+    private static final ExtensionContext.Namespace NAMESPACE = ExtensionContext.Namespace.create(ApplicationComposerExtension.class.getName());
+    private static final SingleApplicationComposerBase BASE = new SingleApplicationComposerBase();
+
+    @Override
+    public void beforeAll(ExtensionContext context) throws Exception {
+
+        if (isPerJvm(context)) {
+            BASE.start(context.getTestClass().orElse(null));
+        } else if (isPerAll(context)) {
+            doStart(context);
+        } else if (isPerDefault(context)) {
+            if (isPerClass(context)) {
+                doStart(context);
+            }
+        }
+
+        if (isPerClass(context)) {
+            doInject(context);
+        }
+    }
+
+    @Override
+    public void afterAll(ExtensionContext context) throws Exception {
+        if (isPerJvm(context) || isPerAll(context)) {
+            doRelease(context);
+        } else if (isPerDefault(context) && isPerClass(context)) {
+            doRelease(context);
+        }
+    }
+
+
+    @Override
+    public void beforeEach(ExtensionContext context) {
+
+        if (isPerEach(context)) {

Review comment:
       Think I'd try to merge tests which would also enable to move the doInject next the doStart which would help to simplify the flow.
   
   Open question: if beforeAll adds something specific in the store then if it is not in the store there it means it is not "all" or "per jvm" so you start and inject there no? Isn't it easier to just test an "all" marker for each impl?

##########
File path: container/openejb-core/src/main/java/org/apache/openejb/junit/jupiter/ApplicationComposerExtension.java
##########
@@ -0,0 +1,155 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.openejb.junit.jupiter;
+
+import org.apache.openejb.OpenEJBRuntimeException;
+import org.apache.openejb.testing.ApplicationComposers;
+import org.apache.openejb.testing.SingleApplicationComposerBase;
+import org.junit.jupiter.api.extension.*;
+
+import java.util.Arrays;
+import java.util.List;
+import java.util.Objects;
+import java.util.Optional;
+
+public class ApplicationComposerExtension extends ApplicationComposerExtensionBase implements BeforeAllCallback, AfterAllCallback, BeforeEachCallback, AfterEachCallback {
+
+    private static final ExtensionContext.Namespace NAMESPACE = ExtensionContext.Namespace.create(ApplicationComposerExtension.class.getName());
+    private static final SingleApplicationComposerBase BASE = new SingleApplicationComposerBase();
+
+    @Override
+    public void beforeAll(ExtensionContext context) throws Exception {
+
+        if (isPerJvm(context)) {
+            BASE.start(context.getTestClass().orElse(null));
+        } else if (isPerAll(context)) {
+            doStart(context);
+        } else if (isPerDefault(context)) {
+            if (isPerClass(context)) {
+                doStart(context);
+            }
+        }
+
+        if (isPerClass(context)) {
+            doInject(context);
+        }
+    }
+
+    @Override
+    public void afterAll(ExtensionContext context) throws Exception {
+        if (isPerJvm(context) || isPerAll(context)) {
+            doRelease(context);
+        } else if (isPerDefault(context) && isPerClass(context)) {
+            doRelease(context);
+        }
+    }
+
+
+    @Override
+    public void beforeEach(ExtensionContext context) {
+
+        if (isPerEach(context)) {
+            doStart(context);
+        } else if (isPerDefault(context) && !isPerClass(context)) {
+            doStart(context);
+        }
+
+        if (!isPerClass(context)) {
+            doInject(context);
+        }
+    }
+
+    @Override
+    public void afterEach(ExtensionContext context) throws Exception {
+        if (isPerEach(context)) {

Review comment:
       I'd use the same AfterEachReleaser hack (side note: name is not great but you get the idea ;)).

##########
File path: container/openejb-core/src/main/java/org/apache/openejb/junit/jupiter/ApplicationComposerExtension.java
##########
@@ -0,0 +1,155 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.openejb.junit.jupiter;
+
+import org.apache.openejb.OpenEJBRuntimeException;
+import org.apache.openejb.testing.ApplicationComposers;
+import org.apache.openejb.testing.SingleApplicationComposerBase;
+import org.junit.jupiter.api.extension.*;
+
+import java.util.Arrays;
+import java.util.List;
+import java.util.Objects;
+import java.util.Optional;
+
+public class ApplicationComposerExtension extends ApplicationComposerExtensionBase implements BeforeAllCallback, AfterAllCallback, BeforeEachCallback, AfterEachCallback {
+
+    private static final ExtensionContext.Namespace NAMESPACE = ExtensionContext.Namespace.create(ApplicationComposerExtension.class.getName());
+    private static final SingleApplicationComposerBase BASE = new SingleApplicationComposerBase();
+
+    @Override
+    public void beforeAll(ExtensionContext context) throws Exception {
+
+        if (isPerJvm(context)) {
+            BASE.start(context.getTestClass().orElse(null));

Review comment:
       do you want to check there is no @Module/@Classes/... or so in the class? it would be a misuse of the API

##########
File path: container/openejb-core/src/main/java/org/apache/openejb/junit/jupiter/ApplicationComposerExtension.java
##########
@@ -0,0 +1,155 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.openejb.junit.jupiter;
+
+import org.apache.openejb.OpenEJBRuntimeException;
+import org.apache.openejb.testing.ApplicationComposers;
+import org.apache.openejb.testing.SingleApplicationComposerBase;
+import org.junit.jupiter.api.extension.*;
+
+import java.util.Arrays;
+import java.util.List;
+import java.util.Objects;
+import java.util.Optional;
+
+public class ApplicationComposerExtension extends ApplicationComposerExtensionBase implements BeforeAllCallback, AfterAllCallback, BeforeEachCallback, AfterEachCallback {
+
+    private static final ExtensionContext.Namespace NAMESPACE = ExtensionContext.Namespace.create(ApplicationComposerExtension.class.getName());
+    private static final SingleApplicationComposerBase BASE = new SingleApplicationComposerBase();
+
+    @Override
+    public void beforeAll(ExtensionContext context) throws Exception {
+
+        if (isPerJvm(context)) {
+            BASE.start(context.getTestClass().orElse(null));
+        } else if (isPerAll(context)) {
+            doStart(context);
+        } else if (isPerDefault(context)) {
+            if (isPerClass(context)) {
+                doStart(context);
+            }
+        }
+
+        if (isPerClass(context)) {
+            doInject(context);
+        }
+    }
+
+    @Override
+    public void afterAll(ExtensionContext context) throws Exception {
+        if (isPerJvm(context) || isPerAll(context)) {

Review comment:
       perjvm => do not release?

##########
File path: container/openejb-core/pom.xml
##########
@@ -580,6 +580,22 @@
       <groupId>org.apache.xbean</groupId>
       <artifactId>xbean-bundleutils</artifactId>
     </dependency>
+    <dependency>

Review comment:
       open point: should it go in a new openejb-junit5 module? Having testing stack in openejb-core is an inheritance but bothering at some point so maybe time to clean it up? wdyt?

##########
File path: tomee/tomee-embedded/src/main/java/org/apache/tomee/embedded/junit/jupiter/TomEEEmbeddedExtension.java
##########
@@ -0,0 +1,86 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.tomee.embedded.junit.jupiter;
+
+import org.apache.openejb.OpenEJBRuntimeException;
+import org.apache.openejb.junit.jupiter.ApplicationComposerExtensionBase;
+import org.apache.tomee.embedded.junit.TomEEEmbeddedBase;
+import org.junit.jupiter.api.TestInstance;
+import org.junit.jupiter.api.extension.*;

Review comment:
       wildcard? ;)

##########
File path: pom.xml
##########
@@ -97,8 +97,7 @@
     <tomee.version>${project.version}</tomee.version>
     <maven.compiler.source>1.8</maven.compiler.source>
     <maven.compiler.target>1.8</maven.compiler.target>
-    <surefire.version>2.21.0</surefire.version>
-    <surefire.junit5.version>3.0.0-M5</surefire.junit5.version>
+    <surefire.version>2.22.1</surefire.version>

Review comment:
       don't downgrade, as mentionned iit can fake it works with junit5 whereas it actually does not

##########
File path: container/openejb-core/src/main/java/org/apache/openejb/junit/jupiter/ApplicationComposerExtension.java
##########
@@ -0,0 +1,155 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.openejb.junit.jupiter;
+
+import org.apache.openejb.OpenEJBRuntimeException;
+import org.apache.openejb.testing.ApplicationComposers;
+import org.apache.openejb.testing.SingleApplicationComposerBase;
+import org.junit.jupiter.api.extension.*;
+
+import java.util.Arrays;
+import java.util.List;
+import java.util.Objects;
+import java.util.Optional;
+
+public class ApplicationComposerExtension extends ApplicationComposerExtensionBase implements BeforeAllCallback, AfterAllCallback, BeforeEachCallback, AfterEachCallback {
+
+    private static final ExtensionContext.Namespace NAMESPACE = ExtensionContext.Namespace.create(ApplicationComposerExtension.class.getName());
+    private static final SingleApplicationComposerBase BASE = new SingleApplicationComposerBase();
+
+    @Override
+    public void beforeAll(ExtensionContext context) throws Exception {
+
+        if (isPerJvm(context)) {
+            BASE.start(context.getTestClass().orElse(null));
+        } else if (isPerAll(context)) {
+            doStart(context);
+        } else if (isPerDefault(context)) {
+            if (isPerClass(context)) {
+                doStart(context);
+            }
+        }
+
+        if (isPerClass(context)) {

Review comment:
       not sure it is intended but previous code makes it possible to not have started anything there and if so it must not inject anything
   I suspect not starting at all is a bug so maybe rework the if/else flow to ensure we start?

##########
File path: examples/junit5-application-composer/pom.xml
##########
@@ -0,0 +1,120 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<!--
+
+    Licensed to the Apache Software Foundation (ASF) under one or more
+    contributor license agreements.  See the NOTICE file distributed with
+    this work for additional information regarding copyright ownership.
+    The ASF licenses this file to You under the Apache License, Version 2.0
+    (the "License"); you may not use this file except in compliance with
+    the License.  You may obtain a copy of the License at
+
+       http://www.apache.org/licenses/LICENSE-2.0
+
+    Unless required by applicable law or agreed to in writing, software
+    distributed under the License is distributed on an "AS IS" BASIS,
+    WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+    See the License for the specific language governing permissions and
+    limitations under the License.
+-->
+<!-- $Rev: 636494 $ $Date: 2008-03-12 21:24:02 +0100 (Wed, 12 Mar 2008) $ -->
+<project xmlns="http://maven.apache.org/POM/4.0.0" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/maven-v4_0_0.xsd">
+  <modelVersion>4.0.0</modelVersion>
+  <groupId>org.superbiz</groupId>
+  <artifactId>junit5-application-composer</artifactId>
+  <packaging>jar</packaging>
+  <version>8.0.7-SNAPSHOT</version>
+  <name>TomEE :: Examples :: JUnit 5 :: Application Composer</name>
+  <properties>
+    <project.build.sourceEncoding>UTF-8</project.build.sourceEncoding>
+  </properties>
+  <build>
+    <defaultGoal>install</defaultGoal>
+    <plugins>
+      <plugin>
+        <groupId>org.apache.maven.plugins</groupId>
+        <artifactId>maven-compiler-plugin</artifactId>
+        <version>3.5.1</version>
+        <configuration>
+          <source>1.8</source>
+          <target>1.8</target>
+        </configuration>
+      </plugin>
+      <plugin>
+        <groupId>org.apache.maven.plugins</groupId>
+        <artifactId>maven-surefire-plugin</artifactId>
+        <version>2.22.1</version>

Review comment:
       use 3.0.0-M5 please, previous versions have all pitfalls with junit5

##########
File path: container/openejb-core/src/main/java/org/apache/openejb/junit/jupiter/ApplicationComposerExtension.java
##########
@@ -0,0 +1,155 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.openejb.junit.jupiter;
+
+import org.apache.openejb.OpenEJBRuntimeException;
+import org.apache.openejb.testing.ApplicationComposers;
+import org.apache.openejb.testing.SingleApplicationComposerBase;
+import org.junit.jupiter.api.extension.*;
+
+import java.util.Arrays;
+import java.util.List;
+import java.util.Objects;
+import java.util.Optional;
+
+public class ApplicationComposerExtension extends ApplicationComposerExtensionBase implements BeforeAllCallback, AfterAllCallback, BeforeEachCallback, AfterEachCallback {
+
+    private static final ExtensionContext.Namespace NAMESPACE = ExtensionContext.Namespace.create(ApplicationComposerExtension.class.getName());
+    private static final SingleApplicationComposerBase BASE = new SingleApplicationComposerBase();
+
+    @Override
+    public void beforeAll(ExtensionContext context) throws Exception {
+
+        if (isPerJvm(context)) {
+            BASE.start(context.getTestClass().orElse(null));
+        } else if (isPerAll(context)) {
+            doStart(context);
+        } else if (isPerDefault(context)) {
+            if (isPerClass(context)) {
+                doStart(context);
+            }
+        }
+
+        if (isPerClass(context)) {
+            doInject(context);
+        }
+    }
+
+    @Override
+    public void afterAll(ExtensionContext context) throws Exception {
+        if (isPerJvm(context) || isPerAll(context)) {
+            doRelease(context);
+        } else if (isPerDefault(context) && isPerClass(context)) {
+            doRelease(context);
+        }
+    }
+
+
+    @Override
+    public void beforeEach(ExtensionContext context) {
+
+        if (isPerEach(context)) {
+            doStart(context);
+        } else if (isPerDefault(context) && !isPerClass(context)) {
+            doStart(context);
+        }
+
+        if (!isPerClass(context)) {
+            doInject(context);
+        }
+    }
+
+    @Override
+    public void afterEach(ExtensionContext context) throws Exception {
+        if (isPerEach(context)) {
+            doRelease(context);
+        } else if (isPerDefault(context) && !isPerClass(context)) {
+            doRelease(context);
+        }
+    }
+
+    private void doRelease(final ExtensionContext extensionContext) throws Exception {
+        if (isPerJvm(extensionContext)) {
+            //FIXME how to release / close after last test class execution?
+            BASE.close();
+        } else {
+            extensionContext.getStore(NAMESPACE).get(ApplicationComposers.class, ApplicationComposers.class).after();
+        }
+    }
+
+    private void doStart(final ExtensionContext extensionContext) {
+
+        Optional<Class<?>> oClazz = extensionContext.getTestClass();
+
+        if (!oClazz.isPresent()) {
+            throw new OpenEJBRuntimeException("Could not get test class from the given extension context.");
+        }
+
+        extensionContext.getStore(NAMESPACE).put(ApplicationComposers.class,
+                new ApplicationComposers(oClazz.get(), getAdditionalModules(oClazz.get())));
+
+    }
+
+    private void doInject(final ExtensionContext extensionContext) {
+        Optional<TestInstances> oTestInstances = extensionContext.getTestInstances();

Review comment:
       orElseThrow ;)

##########
File path: container/openejb-core/src/main/java/org/apache/openejb/junit/jupiter/ApplicationComposerExtension.java
##########
@@ -0,0 +1,155 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.openejb.junit.jupiter;
+
+import org.apache.openejb.OpenEJBRuntimeException;
+import org.apache.openejb.testing.ApplicationComposers;
+import org.apache.openejb.testing.SingleApplicationComposerBase;
+import org.junit.jupiter.api.extension.*;
+
+import java.util.Arrays;
+import java.util.List;
+import java.util.Objects;
+import java.util.Optional;
+
+public class ApplicationComposerExtension extends ApplicationComposerExtensionBase implements BeforeAllCallback, AfterAllCallback, BeforeEachCallback, AfterEachCallback {
+
+    private static final ExtensionContext.Namespace NAMESPACE = ExtensionContext.Namespace.create(ApplicationComposerExtension.class.getName());
+    private static final SingleApplicationComposerBase BASE = new SingleApplicationComposerBase();
+
+    @Override
+    public void beforeAll(ExtensionContext context) throws Exception {
+
+        if (isPerJvm(context)) {
+            BASE.start(context.getTestClass().orElse(null));
+        } else if (isPerAll(context)) {
+            doStart(context);
+        } else if (isPerDefault(context)) {
+            if (isPerClass(context)) {
+                doStart(context);
+            }
+        }
+
+        if (isPerClass(context)) {
+            doInject(context);
+        }
+    }
+
+    @Override
+    public void afterAll(ExtensionContext context) throws Exception {
+        if (isPerJvm(context) || isPerAll(context)) {
+            doRelease(context);
+        } else if (isPerDefault(context) && isPerClass(context)) {
+            doRelease(context);
+        }
+    }
+
+
+    @Override
+    public void beforeEach(ExtensionContext context) {
+
+        if (isPerEach(context)) {
+            doStart(context);
+        } else if (isPerDefault(context) && !isPerClass(context)) {
+            doStart(context);
+        }
+
+        if (!isPerClass(context)) {
+            doInject(context);
+        }
+    }
+
+    @Override
+    public void afterEach(ExtensionContext context) throws Exception {
+        if (isPerEach(context)) {
+            doRelease(context);
+        } else if (isPerDefault(context) && !isPerClass(context)) {
+            doRelease(context);
+        }
+    }
+
+    private void doRelease(final ExtensionContext extensionContext) throws Exception {
+        if (isPerJvm(extensionContext)) {
+            //FIXME how to release / close after last test class execution?
+            BASE.close();
+        } else {
+            extensionContext.getStore(NAMESPACE).get(ApplicationComposers.class, ApplicationComposers.class).after();
+        }
+    }
+
+    private void doStart(final ExtensionContext extensionContext) {
+
+        Optional<Class<?>> oClazz = extensionContext.getTestClass();

Review comment:
       > final Class<?> oClazz =  extensionContext.getTestClass().orElseThrow(() ->new OpenEJBRuntimeException("Could not get test class from the given extension context."));
   

##########
File path: examples/junit5-application-composer/pom.xml
##########
@@ -0,0 +1,120 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<!--
+
+    Licensed to the Apache Software Foundation (ASF) under one or more
+    contributor license agreements.  See the NOTICE file distributed with
+    this work for additional information regarding copyright ownership.
+    The ASF licenses this file to You under the Apache License, Version 2.0
+    (the "License"); you may not use this file except in compliance with
+    the License.  You may obtain a copy of the License at
+
+       http://www.apache.org/licenses/LICENSE-2.0
+
+    Unless required by applicable law or agreed to in writing, software
+    distributed under the License is distributed on an "AS IS" BASIS,
+    WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+    See the License for the specific language governing permissions and
+    limitations under the License.
+-->
+<!-- $Rev: 636494 $ $Date: 2008-03-12 21:24:02 +0100 (Wed, 12 Mar 2008) $ -->
+<project xmlns="http://maven.apache.org/POM/4.0.0" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/maven-v4_0_0.xsd">
+  <modelVersion>4.0.0</modelVersion>
+  <groupId>org.superbiz</groupId>
+  <artifactId>junit5-application-composer</artifactId>
+  <packaging>jar</packaging>
+  <version>8.0.7-SNAPSHOT</version>
+  <name>TomEE :: Examples :: JUnit 5 :: Application Composer</name>
+  <properties>
+    <project.build.sourceEncoding>UTF-8</project.build.sourceEncoding>
+  </properties>
+  <build>
+    <defaultGoal>install</defaultGoal>
+    <plugins>
+      <plugin>
+        <groupId>org.apache.maven.plugins</groupId>
+        <artifactId>maven-compiler-plugin</artifactId>
+        <version>3.5.1</version>
+        <configuration>
+          <source>1.8</source>
+          <target>1.8</target>
+        </configuration>
+      </plugin>
+      <plugin>
+        <groupId>org.apache.maven.plugins</groupId>
+        <artifactId>maven-surefire-plugin</artifactId>
+        <version>2.22.1</version>
+        <configuration>
+          <argLine>-Djdk.attach.allowAttachSelf</argLine>
+        </configuration>
+      </plugin>
+      <plugin>
+        <groupId>org.tomitribe.transformer</groupId>
+        <artifactId>org.eclipse.transformer.maven</artifactId>
+        <version>0.1.1a</version>
+        <configuration>
+          <classifier>jakartaee9</classifier>
+        </configuration>
+        <executions>
+          <execution>
+            <goals>
+              <goal>run</goal>
+            </goals>
+            <phase>package</phase>
+          </execution>
+        </executions>
+      </plugin>
+    </plugins>
+  </build>
+  <repositories>
+    <repository>
+      <id>apache-m2-snapshot</id>
+      <name>Apache Snapshot Repository</name>
+      <url>https://repository.apache.org/content/groups/snapshots</url>
+    </repository>
+  </repositories>
+  <dependencies>
+    <dependency>
+      <groupId>org.apache.tomee</groupId>
+      <artifactId>javaee-api</artifactId>
+      <version>[8.0,)</version>
+      <scope>provided</scope>
+    </dependency>
+    <dependency>
+      <groupId>org.junit.jupiter</groupId>
+      <artifactId>junit-jupiter-api</artifactId>

Review comment:
       <artifactId>junit-jupiter</artifactId> as single junit5 dep

##########
File path: container/openejb-core/src/main/java/org/apache/openejb/junit/jupiter/ApplicationComposerExtension.java
##########
@@ -0,0 +1,155 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.openejb.junit.jupiter;
+
+import org.apache.openejb.OpenEJBRuntimeException;
+import org.apache.openejb.testing.ApplicationComposers;
+import org.apache.openejb.testing.SingleApplicationComposerBase;
+import org.junit.jupiter.api.extension.*;
+
+import java.util.Arrays;
+import java.util.List;
+import java.util.Objects;
+import java.util.Optional;
+
+public class ApplicationComposerExtension extends ApplicationComposerExtensionBase implements BeforeAllCallback, AfterAllCallback, BeforeEachCallback, AfterEachCallback {
+
+    private static final ExtensionContext.Namespace NAMESPACE = ExtensionContext.Namespace.create(ApplicationComposerExtension.class.getName());
+    private static final SingleApplicationComposerBase BASE = new SingleApplicationComposerBase();
+
+    @Override
+    public void beforeAll(ExtensionContext context) throws Exception {
+
+        if (isPerJvm(context)) {
+            BASE.start(context.getTestClass().orElse(null));
+        } else if (isPerAll(context)) {
+            doStart(context);
+        } else if (isPerDefault(context)) {
+            if (isPerClass(context)) {
+                doStart(context);
+            }
+        }
+
+        if (isPerClass(context)) {
+            doInject(context);
+        }
+    }
+
+    @Override
+    public void afterAll(ExtensionContext context) throws Exception {
+        if (isPerJvm(context) || isPerAll(context)) {
+            doRelease(context);
+        } else if (isPerDefault(context) && isPerClass(context)) {

Review comment:
       my 2 cts would be to create an AfterAllReleaser {void run()} and put it in the context in beforeAll next to the related start logic.
   Then afterXXX is just context.getNamespace(...).getStore().get(AfterAllReleaser.class, AfterAllReleaser.class).ifPresent(AfterAllReleaser::run);
   
   Same can apply to afterEach.
   
   Big advantage is you don't duplicate all tests, it is symmetric by construction.

##########
File path: container/openejb-core/src/main/java/org/apache/openejb/junit/jupiter/ApplicationComposerExtension.java
##########
@@ -0,0 +1,155 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.openejb.junit.jupiter;
+
+import org.apache.openejb.OpenEJBRuntimeException;
+import org.apache.openejb.testing.ApplicationComposers;
+import org.apache.openejb.testing.SingleApplicationComposerBase;
+import org.junit.jupiter.api.extension.*;
+
+import java.util.Arrays;
+import java.util.List;
+import java.util.Objects;
+import java.util.Optional;
+
+public class ApplicationComposerExtension extends ApplicationComposerExtensionBase implements BeforeAllCallback, AfterAllCallback, BeforeEachCallback, AfterEachCallback {
+
+    private static final ExtensionContext.Namespace NAMESPACE = ExtensionContext.Namespace.create(ApplicationComposerExtension.class.getName());
+    private static final SingleApplicationComposerBase BASE = new SingleApplicationComposerBase();
+
+    @Override
+    public void beforeAll(ExtensionContext context) throws Exception {
+
+        if (isPerJvm(context)) {
+            BASE.start(context.getTestClass().orElse(null));
+        } else if (isPerAll(context)) {
+            doStart(context);
+        } else if (isPerDefault(context)) {
+            if (isPerClass(context)) {
+                doStart(context);
+            }
+        }
+
+        if (isPerClass(context)) {
+            doInject(context);
+        }
+    }
+
+    @Override
+    public void afterAll(ExtensionContext context) throws Exception {
+        if (isPerJvm(context) || isPerAll(context)) {
+            doRelease(context);
+        } else if (isPerDefault(context) && isPerClass(context)) {
+            doRelease(context);
+        }
+    }
+
+
+    @Override
+    public void beforeEach(ExtensionContext context) {
+
+        if (isPerEach(context)) {
+            doStart(context);
+        } else if (isPerDefault(context) && !isPerClass(context)) {
+            doStart(context);
+        }
+
+        if (!isPerClass(context)) {
+            doInject(context);
+        }
+    }
+
+    @Override
+    public void afterEach(ExtensionContext context) throws Exception {
+        if (isPerEach(context)) {
+            doRelease(context);
+        } else if (isPerDefault(context) && !isPerClass(context)) {
+            doRelease(context);
+        }
+    }
+
+    private void doRelease(final ExtensionContext extensionContext) throws Exception {
+        if (isPerJvm(extensionContext)) {
+            //FIXME how to release / close after last test class execution?
+            BASE.close();
+        } else {
+            extensionContext.getStore(NAMESPACE).get(ApplicationComposers.class, ApplicationComposers.class).after();
+        }
+    }
+
+    private void doStart(final ExtensionContext extensionContext) {
+
+        Optional<Class<?>> oClazz = extensionContext.getTestClass();
+
+        if (!oClazz.isPresent()) {
+            throw new OpenEJBRuntimeException("Could not get test class from the given extension context.");
+        }
+
+        extensionContext.getStore(NAMESPACE).put(ApplicationComposers.class,
+                new ApplicationComposers(oClazz.get(), getAdditionalModules(oClazz.get())));
+
+    }
+
+    private void doInject(final ExtensionContext extensionContext) {
+        Optional<TestInstances> oTestInstances = extensionContext.getTestInstances();
+
+        if (!oTestInstances.isPresent()) {
+            throw new OpenEJBRuntimeException("No test instances available for the given extension context.");
+        }
+
+        List<Object> testInstances = oTestInstances.get().getAllInstances();
+
+        if (isPerJvm(extensionContext)) {
+            testInstances.forEach(t -> {
+                try {
+                    BASE.composerInject(t);
+                } catch (Exception e) {
+                    throw new OpenEJBRuntimeException(e);
+                }
+            });
+        } else {
+            ApplicationComposers delegate = extensionContext.getStore(NAMESPACE)
+                    .get(ApplicationComposers.class, ApplicationComposers.class);
+
+            testInstances.forEach(t -> {
+                try {
+                    delegate.before(t);

Review comment:
       this does not injects but starts a container so this is not supposed to work - or at least as expected, maybe add a test to validate this case

##########
File path: container/openejb-core/src/main/java/org/apache/openejb/junit/jupiter/ApplicationComposerExtension.java
##########
@@ -0,0 +1,155 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.openejb.junit.jupiter;
+
+import org.apache.openejb.OpenEJBRuntimeException;
+import org.apache.openejb.testing.ApplicationComposers;
+import org.apache.openejb.testing.SingleApplicationComposerBase;
+import org.junit.jupiter.api.extension.*;
+
+import java.util.Arrays;
+import java.util.List;
+import java.util.Objects;
+import java.util.Optional;
+
+public class ApplicationComposerExtension extends ApplicationComposerExtensionBase implements BeforeAllCallback, AfterAllCallback, BeforeEachCallback, AfterEachCallback {
+
+    private static final ExtensionContext.Namespace NAMESPACE = ExtensionContext.Namespace.create(ApplicationComposerExtension.class.getName());
+    private static final SingleApplicationComposerBase BASE = new SingleApplicationComposerBase();
+
+    @Override
+    public void beforeAll(ExtensionContext context) throws Exception {
+
+        if (isPerJvm(context)) {
+            BASE.start(context.getTestClass().orElse(null));
+        } else if (isPerAll(context)) {
+            doStart(context);
+        } else if (isPerDefault(context)) {
+            if (isPerClass(context)) {
+                doStart(context);
+            }
+        }
+
+        if (isPerClass(context)) {
+            doInject(context);
+        }
+    }
+
+    @Override
+    public void afterAll(ExtensionContext context) throws Exception {
+        if (isPerJvm(context) || isPerAll(context)) {
+            doRelease(context);
+        } else if (isPerDefault(context) && isPerClass(context)) {
+            doRelease(context);
+        }
+    }
+
+
+    @Override
+    public void beforeEach(ExtensionContext context) {
+
+        if (isPerEach(context)) {
+            doStart(context);
+        } else if (isPerDefault(context) && !isPerClass(context)) {
+            doStart(context);
+        }
+
+        if (!isPerClass(context)) {
+            doInject(context);
+        }
+    }
+
+    @Override
+    public void afterEach(ExtensionContext context) throws Exception {
+        if (isPerEach(context)) {
+            doRelease(context);
+        } else if (isPerDefault(context) && !isPerClass(context)) {
+            doRelease(context);
+        }
+    }
+
+    private void doRelease(final ExtensionContext extensionContext) throws Exception {
+        if (isPerJvm(extensionContext)) {
+            //FIXME how to release / close after last test class execution?
+            BASE.close();
+        } else {
+            extensionContext.getStore(NAMESPACE).get(ApplicationComposers.class, ApplicationComposers.class).after();
+        }
+    }
+
+    private void doStart(final ExtensionContext extensionContext) {
+
+        Optional<Class<?>> oClazz = extensionContext.getTestClass();
+
+        if (!oClazz.isPresent()) {
+            throw new OpenEJBRuntimeException("Could not get test class from the given extension context.");
+        }
+
+        extensionContext.getStore(NAMESPACE).put(ApplicationComposers.class,
+                new ApplicationComposers(oClazz.get(), getAdditionalModules(oClazz.get())));
+
+    }
+
+    private void doInject(final ExtensionContext extensionContext) {
+        Optional<TestInstances> oTestInstances = extensionContext.getTestInstances();
+
+        if (!oTestInstances.isPresent()) {
+            throw new OpenEJBRuntimeException("No test instances available for the given extension context.");
+        }
+
+        List<Object> testInstances = oTestInstances.get().getAllInstances();
+
+        if (isPerJvm(extensionContext)) {
+            testInstances.forEach(t -> {
+                try {
+                    BASE.composerInject(t);
+                } catch (Exception e) {
+                    throw new OpenEJBRuntimeException(e);
+                }
+            });
+        } else {
+            ApplicationComposers delegate = extensionContext.getStore(NAMESPACE)
+                    .get(ApplicationComposers.class, ApplicationComposers.class);
+
+            testInstances.forEach(t -> {
+                try {
+                    delegate.before(t);
+                } catch (Exception e) {
+                    throw new OpenEJBRuntimeException(e);
+                }
+            });
+        }
+    }
+
+    private Object[] getAdditionalModules(final Class<?> clazz) {
+        return Arrays.stream(clazz.getClasses())

Review comment:
       this likely do not do what is expected and can be random (thinking to @Nested tests of junit5).
   
   ApplicationComposerRule shows the idea: you pass the list of additional modules.
   
   In junit5 it can be:
   
   > @RegisterExtension ApplicationComposerExtension ext = new ApplicationComposerExtension(new MyApp());
   
   it enables to define the app outside the test and/or to compose subapps definitions (like a MyStack() + finish the spec in the test)

##########
File path: tomee/tomee-embedded/src/main/java/org/apache/tomee/embedded/junit/jupiter/TomEEEmbeddedExtension.java
##########
@@ -0,0 +1,86 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.tomee.embedded.junit.jupiter;
+
+import org.apache.openejb.OpenEJBRuntimeException;
+import org.apache.openejb.junit.jupiter.ApplicationComposerExtensionBase;
+import org.apache.tomee.embedded.junit.TomEEEmbeddedBase;
+import org.junit.jupiter.api.TestInstance;
+import org.junit.jupiter.api.extension.*;
+
+import java.util.List;
+import java.util.Optional;
+
+public class TomEEEmbeddedExtension extends ApplicationComposerExtensionBase implements BeforeAllCallback, AfterAllCallback, BeforeEachCallback, AfterEachCallback {

Review comment:
       maybe also define the @RunWithTomEEEmbedded
   
   Also wonder if ApplicationComposerExtensionBase is needed, as seen in app composer case, you have beforeAll to test if you are  in JVM or per class mode, then all other tests are deduced from the content of the context store so not sure it is good to define a dependency between  both modules just to test a scope. (from an artifact/dep point of view we should be able to run tomee embedded without application composer impl, only its org.apache.openejb.testing API)

##########
File path: container/openejb-core/src/main/java/org/apache/openejb/junit/jupiter/ApplicationComposerExtension.java
##########
@@ -0,0 +1,155 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.openejb.junit.jupiter;
+
+import org.apache.openejb.OpenEJBRuntimeException;
+import org.apache.openejb.testing.ApplicationComposers;
+import org.apache.openejb.testing.SingleApplicationComposerBase;
+import org.junit.jupiter.api.extension.*;
+
+import java.util.Arrays;
+import java.util.List;
+import java.util.Objects;
+import java.util.Optional;
+
+public class ApplicationComposerExtension extends ApplicationComposerExtensionBase implements BeforeAllCallback, AfterAllCallback, BeforeEachCallback, AfterEachCallback {
+
+    private static final ExtensionContext.Namespace NAMESPACE = ExtensionContext.Namespace.create(ApplicationComposerExtension.class.getName());
+    private static final SingleApplicationComposerBase BASE = new SingleApplicationComposerBase();
+
+    @Override
+    public void beforeAll(ExtensionContext context) throws Exception {
+
+        if (isPerJvm(context)) {
+            BASE.start(context.getTestClass().orElse(null));
+        } else if (isPerAll(context)) {
+            doStart(context);
+        } else if (isPerDefault(context)) {
+            if (isPerClass(context)) {
+                doStart(context);
+            }
+        }
+
+        if (isPerClass(context)) {
+            doInject(context);
+        }
+    }
+
+    @Override
+    public void afterAll(ExtensionContext context) throws Exception {
+        if (isPerJvm(context) || isPerAll(context)) {
+            doRelease(context);
+        } else if (isPerDefault(context) && isPerClass(context)) {
+            doRelease(context);
+        }
+    }
+
+
+    @Override
+    public void beforeEach(ExtensionContext context) {
+
+        if (isPerEach(context)) {
+            doStart(context);
+        } else if (isPerDefault(context) && !isPerClass(context)) {
+            doStart(context);
+        }
+
+        if (!isPerClass(context)) {
+            doInject(context);
+        }
+    }
+
+    @Override
+    public void afterEach(ExtensionContext context) throws Exception {
+        if (isPerEach(context)) {
+            doRelease(context);
+        } else if (isPerDefault(context) && !isPerClass(context)) {
+            doRelease(context);
+        }
+    }
+
+    private void doRelease(final ExtensionContext extensionContext) throws Exception {
+        if (isPerJvm(extensionContext)) {
+            //FIXME how to release / close after last test class execution?
+            BASE.close();
+        } else {
+            extensionContext.getStore(NAMESPACE).get(ApplicationComposers.class, ApplicationComposers.class).after();
+        }
+    }
+
+    private void doStart(final ExtensionContext extensionContext) {
+
+        Optional<Class<?>> oClazz = extensionContext.getTestClass();
+
+        if (!oClazz.isPresent()) {
+            throw new OpenEJBRuntimeException("Could not get test class from the given extension context.");
+        }
+
+        extensionContext.getStore(NAMESPACE).put(ApplicationComposers.class,
+                new ApplicationComposers(oClazz.get(), getAdditionalModules(oClazz.get())));
+
+    }
+
+    private void doInject(final ExtensionContext extensionContext) {
+        Optional<TestInstances> oTestInstances = extensionContext.getTestInstances();
+
+        if (!oTestInstances.isPresent()) {
+            throw new OpenEJBRuntimeException("No test instances available for the given extension context.");
+        }
+
+        List<Object> testInstances = oTestInstances.get().getAllInstances();
+
+        if (isPerJvm(extensionContext)) {
+            testInstances.forEach(t -> {
+                try {
+                    BASE.composerInject(t);
+                } catch (Exception e) {
+                    throw new OpenEJBRuntimeException(e);
+                }
+            });
+        } else {
+            ApplicationComposers delegate = extensionContext.getStore(NAMESPACE)
+                    .get(ApplicationComposers.class, ApplicationComposers.class);
+
+            testInstances.forEach(t -> {
+                try {
+                    delegate.before(t);
+                } catch (Exception e) {
+                    throw new OpenEJBRuntimeException(e);
+                }
+            });
+        }
+    }
+
+    private Object[] getAdditionalModules(final Class<?> clazz) {
+        return Arrays.stream(clazz.getClasses())
+                .map(this::newInstance)
+                .filter(Objects::nonNull)
+                .toArray();
+    }
+
+    private Object newInstance(final Class<?> clazz) {
+        try {
+            return clazz.newInstance();

Review comment:
       > clazz.getConstructor().newInstance()
   
   blame java, not me ;)
   
   (only relevant if you decide to still use nested classes which is not needed regarding previous comment)




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]


Reply | Threaded
Open this post in threaded view
|

[GitHub] [tomee] rzo1 commented on a change in pull request #767: TOMEE-2977 - Provide a ApplicationComposer JUnit 5 Extension

GitBox
In reply to this post by GitBox

rzo1 commented on a change in pull request #767:
URL: https://github.com/apache/tomee/pull/767#discussion_r595487810



##########
File path: tomee/tomee-embedded/src/main/java/org/apache/tomee/embedded/junit/jupiter/TomEEEmbeddedExtension.java
##########
@@ -0,0 +1,86 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.tomee.embedded.junit.jupiter;
+
+import org.apache.openejb.OpenEJBRuntimeException;
+import org.apache.openejb.junit.jupiter.ApplicationComposerExtensionBase;
+import org.apache.tomee.embedded.junit.TomEEEmbeddedBase;
+import org.junit.jupiter.api.TestInstance;
+import org.junit.jupiter.api.extension.*;
+
+import java.util.List;
+import java.util.Optional;
+
+public class TomEEEmbeddedExtension extends ApplicationComposerExtensionBase implements BeforeAllCallback, AfterAllCallback, BeforeEachCallback, AfterEachCallback {

Review comment:
       Good catch. `ApplicationComposerExtensionBase` isn't needed nor is it used.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]


Reply | Threaded
Open this post in threaded view
|

[GitHub] [tomee] rzo1 commented on a change in pull request #767: TOMEE-2977 - Provide a ApplicationComposer JUnit 5 Extension

GitBox
In reply to this post by GitBox

rzo1 commented on a change in pull request #767:
URL: https://github.com/apache/tomee/pull/767#discussion_r595489470



##########
File path: container/openejb-core/pom.xml
##########
@@ -580,6 +580,22 @@
       <groupId>org.apache.xbean</groupId>
       <artifactId>xbean-bundleutils</artifactId>
     </dependency>
+    <dependency>

Review comment:
       I think it would be (a) cleaner and (b) could allow user's to exclude the compile dependencies towards junit4  - so +1 for `openejb-junit5`.
   
   Could also include the classes from `openejb-junit5-backward`, so we have all junit 5 related things in one module. wdyt?




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]


Reply | Threaded
Open this post in threaded view
|

[GitHub] [tomee] rzo1 commented on a change in pull request #767: TOMEE-2977 - Provide a ApplicationComposer JUnit 5 Extension

GitBox
In reply to this post by GitBox

rzo1 commented on a change in pull request #767:
URL: https://github.com/apache/tomee/pull/767#discussion_r595489470



##########
File path: container/openejb-core/pom.xml
##########
@@ -580,6 +580,22 @@
       <groupId>org.apache.xbean</groupId>
       <artifactId>xbean-bundleutils</artifactId>
     </dependency>
+    <dependency>

Review comment:
       I think it would be (a) cleaner and (b) could allow user's to exclude the compile dependencies towards junit4  - so +1 for `openejb-junit5`.
   
   Could also include the classes from `openejb-junit5-backward` (was not released yet -> so no problem in renaming / dropping it), so we have all junit 5 related things in one module. wdyt?




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]


Reply | Threaded
Open this post in threaded view
|

[GitHub] [tomee] rzo1 commented on a change in pull request #767: TOMEE-2977 - Provide a ApplicationComposer JUnit 5 Extension

GitBox
In reply to this post by GitBox

rzo1 commented on a change in pull request #767:
URL: https://github.com/apache/tomee/pull/767#discussion_r595907948



##########
File path: container/openejb-core/src/main/java/org/apache/openejb/junit/jupiter/ApplicationComposerExtension.java
##########
@@ -0,0 +1,155 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.openejb.junit.jupiter;
+
+import org.apache.openejb.OpenEJBRuntimeException;
+import org.apache.openejb.testing.ApplicationComposers;
+import org.apache.openejb.testing.SingleApplicationComposerBase;
+import org.junit.jupiter.api.extension.*;
+
+import java.util.Arrays;
+import java.util.List;
+import java.util.Objects;
+import java.util.Optional;
+
+public class ApplicationComposerExtension extends ApplicationComposerExtensionBase implements BeforeAllCallback, AfterAllCallback, BeforeEachCallback, AfterEachCallback {
+
+    private static final ExtensionContext.Namespace NAMESPACE = ExtensionContext.Namespace.create(ApplicationComposerExtension.class.getName());
+    private static final SingleApplicationComposerBase BASE = new SingleApplicationComposerBase();
+
+    @Override
+    public void beforeAll(ExtensionContext context) throws Exception {
+
+        if (isPerJvm(context)) {
+            BASE.start(context.getTestClass().orElse(null));

Review comment:
       Couldn't find any documentation related to `@Application` at all. It seems, it isn't quite often used in the code base nor in the examples?
   
   `ApplicationComposers#validate()`will complain and fail to start any container, if there is no `@Classes` or `@Default` annotation present?
   
   Probably, I don't get the point here?




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]


Reply | Threaded
Open this post in threaded view
|

[GitHub] [tomee] rzo1 commented on a change in pull request #767: TOMEE-2977 - Provide a ApplicationComposer JUnit 5 Extension

GitBox
In reply to this post by GitBox

rzo1 commented on a change in pull request #767:
URL: https://github.com/apache/tomee/pull/767#discussion_r595909860



##########
File path: container/openejb-core/src/main/java/org/apache/openejb/junit/jupiter/ApplicationComposerExtension.java
##########
@@ -0,0 +1,155 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.openejb.junit.jupiter;
+
+import org.apache.openejb.OpenEJBRuntimeException;
+import org.apache.openejb.testing.ApplicationComposers;
+import org.apache.openejb.testing.SingleApplicationComposerBase;
+import org.junit.jupiter.api.extension.*;
+
+import java.util.Arrays;
+import java.util.List;
+import java.util.Objects;
+import java.util.Optional;
+
+public class ApplicationComposerExtension extends ApplicationComposerExtensionBase implements BeforeAllCallback, AfterAllCallback, BeforeEachCallback, AfterEachCallback {
+
+    private static final ExtensionContext.Namespace NAMESPACE = ExtensionContext.Namespace.create(ApplicationComposerExtension.class.getName());
+    private static final SingleApplicationComposerBase BASE = new SingleApplicationComposerBase();
+
+    @Override
+    public void beforeAll(ExtensionContext context) throws Exception {
+
+        if (isPerJvm(context)) {

Review comment:
       I updated the related code fragments (via `validate(context)`. I don't know exactly, if I got the idea. Can you check, @rmannibucau ?




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]


123