-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Document order of callback methods and extensions that run after test/container execution #1620
Comments
That looks correct to me ... it's assumed that |
Thank for your comment @smoyer64, but I can't agree with you, because you're showing the diagram which is related to user-provided test and lifecycle methods and it looks like expected behavior for me as well. But when I'm extending some test class with extension, and I'm declaring them in a specific order, I'm expecting that they will be invoked in the same order, I mean I understand that for more than one extension order, execution order logic will be the same as for user-provided test and lifecycle methods execution order logic. But, do you think, that this statement is correct "will be executed in the order in which they are declared in the source code."? Maybe it should be changed to avoid misleading or maybe I've understood it wrong. |
@apodkutin Thanks for reporting this issue! @smoyer64 described our intention correctly: Extensions that run after a test are run in reverse order. However, as you noticed that's missing from the documentation and we should add it there. |
Please note that the wrapping behavior for "after" extensions applies to callback methods as well (e.g., I've updated the title of this issue accordingly. |
I've added a Deliverables section as well. |
@smoyer64 , @marcphilipp @sbrannen, thank you guys for the quick answers! But I still have only one question, do you think that this behavior and I'm talking only about callback methods from extensions should be like this? I have one example when I've faced it and why I actually raised this ticket. Example: We have Order of the extensions is
As you already understood, My question is, why you've chosen this approach for execution order of callback extension's methods?
Thank you in advance. |
I've never had a situation where I didn't want to invert the test setup when performing the tear-down. In my mind, the phrase
immediately raised red-flags. In my opinion, you really shouldn't be making assertions outside of test methods. In the case where I've written tests for an extension that implements both a |
I agree with you @smoyer64 that it is a better way to test your custom extensions. But main thoughts were not about approach of testing your extensions, it was just an example to give us food for thought. I'm just wondering maybe there are other cases where you need the same strict declared order of execution's callback methods calls, not only for |
Output generated...
Generated by executing: class WrappingCallbacksAndExtensionsDemo {
@Nested
@ExtendWith({ ExtensionAlpha.class, ExtensionOmega.class})
class TestClass {
@BeforeEach
void setUpA() {
System.out.println(" @BeforeEach " + getClass().getSimpleName() + ".setUpA()");
}
@AfterEach
void tearDownA() {
System.out.println(" @AfterEach " + getClass().getSimpleName() + ".tearDownA()");
}
@BeforeEach
void setUpB() {
System.out.println(" @BeforeEach " + getClass().getSimpleName() + ".setUpB()");
}
@AfterEach
void tearDownB() {
System.out.println(" @AfterEach " + getClass().getSimpleName() + ".tearDownB()");
}
@Test
void test() {
System.out.println("--> " + getClass().getSimpleName() + ".test()");
}
}
static class ExtensionAlpha implements BeforeEachCallback, AfterEachCallback {
public void beforeEach(ExtensionContext context) {
System.out.println(getClass().getSimpleName() + ".beforeEach()");
}
public void afterEach(ExtensionContext context) {
System.out.println(getClass().getSimpleName() + ".afterEach()");
}
}
static class ExtensionOmega implements BeforeEachCallback, AfterEachCallback {
public void beforeEach(ExtensionContext context) {
System.out.println(getClass().getSimpleName() + ".beforeEach()");
}
public void afterEach(ExtensionContext context) {
System.out.println(getClass().getSimpleName() + ".afterEach()");
}
}
} Before I start creating a diagram, is this what we want to document? Anything missing?
|
I think we should add a super class with lifecycle callbacks. |
I agree. |
If that's the actual output, then that demonstrates that we have a potential 🐛 in the support for user-supplied lifecycle callbacks.
|
In terms of naming, I would suggest we not use "alpha" and "omega", since not all cultures are familiar with those Greek letters. Instead, I would simply go with "1" and "2" for simplicity and universal clarity. |
We do actually have proper wrapping support for user-supplied lifecycle callbacks within class hierarchies. For example, the following... @ExtendWith({ ExtensionAlpha.class, ExtensionOmega.class })
static class TestClassA {
@BeforeEach
void setUpA() {
System.out.println(" @BeforeEach " + getClass().getSimpleName() + ".setUpA()");
}
@AfterEach
void tearDownA() {
System.out.println(" @AfterEach " + getClass().getSimpleName() + ".tearDownA()");
}
}
static class TestClassB extends TestClassA {
@BeforeEach
void setUpB() {
System.out.println(" @BeforeEach " + getClass().getSimpleName() + ".setUpB()");
}
@AfterEach
void tearDownB() {
System.out.println(" @AfterEach " + getClass().getSimpleName() + ".tearDownB()");
}
@Test
void testB() {
System.out.println("--> " + getClass().getSimpleName() + ".testB()");
}
} ... results in the following output:
|
Though... we should probably not use the "current class" for such purposes: Thus, it's probably better to use the declaring class/interface for such purposes -- for example, |
Or maybe it's better to just omit the simple class name. 🤔 |
Coming back to that... I now recall the team decision for that behavior. We guarantee wrapping behavior within class hierarchies for user-supplied lifecycle callbacks (e.g., To the casual reader, it would appear that we invoke such methods in alphabetical order. However, that is not precisely true. So we should document that ordering analogous to the existing documentation for
In addition, and more importantly, we need to document that we do not support wrapping behavior for multiple lifecycle methods declared within a single test class or test interface. |
Or.... we could fix this. For example, given the following... import org.junit.jupiter.api.AfterEach;
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.TestInfo;
import org.junit.jupiter.api.extension.AfterEachCallback;
import org.junit.jupiter.api.extension.BeforeEachCallback;
import org.junit.jupiter.api.extension.ExtendWith;
import org.junit.jupiter.api.extension.ExtensionContext;
class WrappingCallbacksAndExtensionsDemo {
@ExtendWith({ Extension1.class, Extension2.class })
static class TestClassA {
private final String className = TestClassA.class.getSimpleName();
@BeforeEach
void setUpA1(TestInfo testInfo) {
System.out.println(" @BeforeEach " + className + ".setUpA1()");
}
@AfterEach
void tearDownA1() {
System.out.println(" @AfterEach " + className + ".tearDownA1()");
}
@BeforeEach
void setUpA2(TestInfo testInfo) {
System.out.println(" @BeforeEach " + className + ".setUpA2()");
}
@AfterEach
void tearDownA2() {
System.out.println(" @AfterEach " + className + ".tearDownA2()");
}
}
static class TestClassB extends TestClassA {
private final String className = TestClassB.class.getSimpleName();
@BeforeEach
void setUpB1(TestInfo testInfo) {
System.out.println(" @BeforeEach " + className + ".setUpB1()");
}
@AfterEach
void tearDownB1() {
System.out.println(" @AfterEach " + className + ".tearDownB1()");
}
@BeforeEach
void setUpB2(TestInfo testInfo) {
System.out.println(" @BeforeEach " + className + ".setUpB2()");
}
@AfterEach
void tearDownB2() {
System.out.println(" @AfterEach " + className + ".tearDownB2()");
}
@BeforeEach
void setUpB3(TestInfo testInfo) {
System.out.println(" @BeforeEach " + className + ".setUpB3()");
}
@AfterEach
void tearDownB3() {
System.out.println(" @AfterEach " + className + ".tearDownB3()");
}
@Test
void testB() {
System.out.println("--> " + className + ".testB()");
}
}
static class TestClassC extends TestClassB {
private final String className = TestClassC.class.getSimpleName();
@BeforeEach
void setUpC1(TestInfo testInfo) {
System.out.println(" @BeforeEach " + className + ".setUpC1()");
}
@AfterEach
void tearDownC1() {
System.out.println(" @AfterEach " + className + ".tearDownC1()");
}
@BeforeEach
void setUpC2(TestInfo testInfo) {
System.out.println(" @BeforeEach " + className + ".setUpC2()");
}
@AfterEach
void tearDownC2() {
System.out.println(" @AfterEach " + className + ".tearDownC2()");
}
@BeforeEach
void setUpC3(TestInfo testInfo) {
System.out.println(" @BeforeEach " + className + ".setUpC3()");
}
@AfterEach
void tearDownC3() {
System.out.println(" @AfterEach " + className + ".tearDownC3()");
}
}
static class Extension1 implements BeforeEachCallback, AfterEachCallback {
@Override
public void beforeEach(ExtensionContext context) {
System.out.println(getClass().getSimpleName() + ".beforeEach()");
}
@Override
public void afterEach(ExtensionContext context) {
System.out.println(getClass().getSimpleName() + ".afterEach()");
}
}
static class Extension2 implements BeforeEachCallback, AfterEachCallback {
@Override
public void beforeEach(ExtensionContext context) {
System.out.println(getClass().getSimpleName() + ".beforeEach()");
}
@Override
public void afterEach(ExtensionContext context) {
System.out.println(getClass().getSimpleName() + ".afterEach()");
}
}
} ... with a minor change I've made to
@junit-team/junit-lambda, Thoughts? |
My local fix works with interface default methods as well. Declaring: class TestClassC extends TestClassB implements TestInterfaceX {
// ...
} ... with the following test interface interface TestInterfaceX {
String className = TestInterfaceX.class.getSimpleName();
@BeforeEach
default void setUpX1(TestInfo testInfo) {
System.out.println(" @BeforeEach " + className + ".setUpX1()");
}
@AfterEach
default void tearDownX1() {
System.out.println(" @AfterEach " + className + ".tearDownX1()");
}
@BeforeEach
default void setUpX2(TestInfo testInfo) {
System.out.println(" @BeforeEach " + className + ".setUpX2()");
}
@AfterEach
default void tearDownX2() {
System.out.println(" @AfterEach " + className + ".tearDownC2()");
}
} ... yields the following output:
|
Example in favor of my "final point" above: @ExtendWith({ Extension1.class, Extension2.class })
static class TestClassZ {
private final String className = TestClassB.class.getSimpleName();
@BeforeEach
void startDatabase(TestInfo testInfo) {
System.out.println(" @BeforeEach " + className + ".startDatabase()");
}
@AfterEach
void closeDatabase() {
System.out.println(" @AfterEach " + className + ".closeDatabase()");
}
@BeforeEach
void setUpZ2(TestInfo testInfo) {
System.out.println(" @BeforeEach " + className + ".setUpZ2()");
}
@AfterEach
void tearDownZ2() {
System.out.println(" @AfterEach " + className + ".tearDownZ2()");
}
@BeforeEach
void setUpZ3(TestInfo testInfo) {
System.out.println(" @BeforeEach " + className + ".setUpZ3()");
}
@AfterEach
void tearDownZ3() {
System.out.println(" @AfterEach " + className + ".tearDownZ3()");
}
@Test
void testZ() {
System.out.println("--> " + className + ".testZ()");
}
} Executing
And that demonstrates that there is in fact zero correlation between methods like Long story, short: let's just document the status quo for execution order for user-supplied lifecycle methods. 😉 |
I'm pasting the "fix" I experimented with locally here for future reference in case it proves useful. Change in /**
* {@link Comparator} that reverses the order of methods declared within the
* same class.
*/
Comparator<Method> lifecycleMethodComparator = (Method m1, Method m2) -> {
Class<?> class1 = m1.getDeclaringClass();
Class<?> class2 = m2.getDeclaringClass();
if (class1.equals(class2)) {
return -1;
}
return 0;
};
private void registerAfterEachMethodAdapters(ExtensionRegistry registry) {
// Make a local copy since findAfterEachMethods() returns an immutable list.
List<Method> afterEachMethods = new ArrayList<>(findAfterEachMethods(this.testClass));
// Since the bottom-up ordering of afterEachMethods will later be reversed when the
// synthesized AfterEachMethodAdapters are executed within TestMethodTestDescriptor,
// we have to reverse the afterEachMethods list to put them in top-down order before
// we register them as synthesized extensions.
Collections.reverse(afterEachMethods);
// Reverse the order of @AfterEach methods declared within the same class/interface.
afterEachMethods.sort(lifecycleMethodComparator);
registerMethodsAsExtensions(afterEachMethods, registry, this::synthesizeAfterEachMethodAdapter);
} |
👍 |
Update: Deliverables have been overhauled to reflect the current scope. |
Examples introduced in 591e5c0 demonstrate proper and improper lifecycle method configuration. Note, however, that that commit only contains the code: it doesn't update the User Guide or Javadoc. |
Executing
|
Executing
|
@sormuras and @marcphilipp, with regard to the undesirable behavior in
|
Copy and paste them to https://app.zenuml.com to generate PNGs. Issue #1620
Based on JUnit5 user-guide extensions execution order should correlate with extensions declaration order:
But it does not.
Example: https://github.com/apodkutin/junit5-extensions-order-test/blob/master/src/test/java/junit5/extensions/order/JUnit5ExtensionsOrderTest.java
@ExtendWith({FirstExtension.class, SecondExtension.class})
Expected output
FirstExtension afterEach method call
SecondExtension afterEach method call
Actual output
SecondExtension afterEach method call
FirstExtension afterEach method call
Jupiter-api version is 5.3.1
Deliverables
Document the lack of explicit ordering for all user-supplied callback methods (i.e.,
@BeforeEach
,@AfterEach
,@BeforeAll
,@AfterAll
) declared within a single test class/interface.Document the lack of wrapping behavior for all user-supplied callback methods (e.g.,
@AfterAll
,@AfterEach
) declared within a single test class/interface.Document wrapping behavior for all user-supplied callback methods (i.e.,
@BeforeEach
,@AfterEach
,@BeforeAll
,@AfterAll
) within test class/interface hierarchies.Document wrapping behavior for all applicable extension APIs (e.g.,
TestExecutionExceptionHandler
,AfterTestExecutionCallback
,AfterEachCallback
,AfterAllCallback
, etc.).The text was updated successfully, but these errors were encountered: