Add NotWorking test extension (#551 / #546)

[Marcono1234]

Preliminary remark: I wanted to implement this either way, therefore I directly created a pull request. However, if you prefer I can also create a GitHub issue where this can be discussed first.

Adds an extension with annotation @NotWorking for marking test methods which are 'not working'; quoting from the documentation added by this pull request:

Often tests fail due to a bug in the tested application or in used dependencies. Traditionally such a test method would be annotated with JUnit's @Disabled. However, this has the following disadvantages when the bug causing the test failure is fixed:

• the developer might not notice the existing test method and create a new one
• the existing test method might not be noticed and remains disabled for a long time after the bug has been fixed, adding no value for the project

@NotWorking tries to solve these issues. Unlike @Disabled it still executes the annotated test method but aborts the test if a test failure or error occurs. However, if the test is executed successfully the @NotWorking annotation will cause a test failure because the test is working. This lets the developer know that they have fixed a bug (possibly by accident) and that they can now remove the @NotWorking annotation from the test method.

Originally I proposed this in junit-team/junit5#2023 (comment), but I am not sure if by accident I 'hijacked' that issue, and whether I described my intentions there clear enough.
Therefore I first wanted to propose this here because I hope it has higher chances being accepted here.

I am not very familiar with JUnit extension development yet, therefore I am not sure if the way I implemented this is a clean way or if it can be improved. Any feedback is appreciated!

Proposed commit message:

Add NotWorking test extension (#551 / #546)

PR: #546

---

PR checklist

The following checklist shall help the PR's author, the reviewers and maintainers to ensure the quality of this project.
It is based on our contributors guidelines, especially the "writing code" section.
It shall help to check for completion of the listed points.
If a point does not apply to the given PR's changes, the corresponding entry can be simply marked as done.

Documentation (general)

• There is documentation (Javadoc and site documentation; added or updated)
• There is implementation information to describe why a non-obvious source code / solution got implemented
• Site documentation has its own .adoc file in the docs folder, e.g. docs/report-entries.adoc
• Only one sentence per line (especially in .adoc files)
• Javadoc uses formal style, while sites documentation may use informal style

Documentation (new extension)

• The docs/docs-nav.yml navigation has an entry for the new extension
• The package-info.java contains information about the new extension

Code

• Code adheres to code style, naming conventions etc.
• Successful tests cover all changes
• There are checks which validate correct / false usage / configuration of a functionality and there are tests to verify those checks
• Tests use AssertJ or our own PioneerAssert (which are based on AssertJ)

Contributing

• A prepared commit message exists
• The list of contributions inside README.md mentions the new contribution (real name optional)

---

I hereby agree to the terms of the JUnit Pioneer Contributor License Agreement.

[Michael1993]

Could you tell me what the purpose of the extension is? What is the difference between a failing test running but not breaking and a test not running at all?

[Marcono1234]

What is the difference between a failing test running but not breaking and a test not running at all?

Let's imagine there is a bug in your application:

1. You mark the corresponding test method with @Disabled
2. At some later point you fix the bug (possibly by accident, e.g. due to refactoring)
3. The test is still disabled, because you are not aware of its existence or you forgot that you disabled it
4. In the worst case you write a new test which covers the exact same scenario covered by the disabled test; this adds maintenance burden

With the proposed @NotWorking extension you would immediately after the bug was fixed (point 2 in the list above) notice the existing test. This avoids

• duplicating tests
• having disabled tests lingering around for a long time after the issue why they were disabled has been solved

[Michael1993]

Don't really see the thought process behind this statement. I thought this value can never be null because for the extension to get invoked during the afterEach phase it must have been invoked during the beforeEach phase.

[Michael1993]

Oh, right - this is because of assumptions? In which case it makes me wonder why aren't you storing the exception object itself instead of a boolean?

[Marcono1234]

Currently both handleTestExecutionException and handleAfterEachMethodExecutionException check the exception type and for test abort exceptions directly throw them.

If the exception object was put in the store, the abort exception check would have to be repeated in the afterEach method. I am not sure if removing the check from handleTestExecutionException and handleAfterEachMethodExecutionException would be a good idea, maybe JUnit behaves differently depending on from where an exception is thrown (i.e. handleTestExecutionException, handleAfterEachMethodExecutionException or afterEach).

I have added some documentation to the EXCEPTION_OCCURRED_STORE_KEY field, does that help? Or would you still prefer to store the exception object?

[Michael1993]

To be honest, I'd inline this method, but that's just my personal preference.

[Marcono1234]

If you don't mind I would like to keep the method for now to avoid having to repeat the getStore(NAMESPACE) part, which is a bit more verbose.

[Bukama]

We talked about this at our last team meeting and want you to invite to take part in a discussion to think about this and similar use cases in a more global view.

Feel free to write down your thoughts and ideas in #550 - thank you!

[Bukama]

Thank you for the PR.
I really like the documentation text about when to use it and especially when not. Maybe a link to assertThrows can be added.

@Michael1993 has already stated his thoughts about the technical implementation.

One minor formal thing: Some time ago we decided that we always want to have an issue on which PRs (like this) are based on to avoid discussing non-implementation related things in the issue instread in the PR.

edit: I've created #551 for this.

[Marcono1234]

Thanks a lot to all of you for being so welcoming and providing such helpful review comments (here and on my other pull requests)! I have tried to address most of the feedback, but left a comment for the refactoring of the value stored in the JUnit Store.

I also don't mind if you want to keep this pull request open until the discussion in #551 has come to a conclusion.

Happy Christmas Holidays! (and no hurry to respond to my comment)

[nipafx]

Thank you @Marcono1234, good work! It's great to see PRs that cover all the bases (from code to tests to documentation, etc.). There are a bunch of comments below. I'm also not too happy with the name - we can discuss that on the issue.

[nipafx]

I was surprised by how many extension points are involved here and checked the existing extension points, looking for a simpler approach. I think I found it with InvocationInterceptor, where all you need to do is proceed with the invocation and then invert the result, i.e. fail on a successful execution and abort on failure. Here's a proof of concept:

import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.extension.ExtendWith;
import org.junit.jupiter.api.extension.ExtensionContext;
import org.junit.jupiter.api.extension.InvocationInterceptor;
import org.junit.jupiter.api.extension.ReflectiveInvocationContext;
import org.opentest4j.AssertionFailedError;
import org.opentest4j.TestAbortedException;

import java.lang.reflect.Method;

import static org.junit.jupiter.api.Assertions.fail;

public class LabTests {

	@Test
	@ExtendWith(InvertResultExtension.class)
	void test() {
		System.out.println("Hi!");
		fail();
	}

	private static class InvertResultExtension implements InvocationInterceptor {

		@Override
		public void interceptTestMethod(Invocation<Void> invocation, ReflectiveInvocationContext<Method> invocationContext, ExtensionContext extensionContext) throws Throwable {
			invert(invocation);
		}

		@Override
		public <T> T interceptTestFactoryMethod(Invocation<T> invocation, ReflectiveInvocationContext<Method> invocationContext, ExtensionContext extensionContext) throws Throwable {
			return invert(invocation);
		}

		private <T> T invert(Invocation<T> invocation) {
			try {
				invocation.proceed();
			} catch (Throwable ex) {
				throw new TestAbortedException("Test failed as expected.", ex);
			}
			throw new AssertionFailedError("Test unexpectedly passed.");
		}

	}

}

[Marcono1234]

That seems to work for nearly all cases except for exceptions thrown in an @AfterEach method, which would then probably require similarly complex logic as it is currently implemented.

Should I switch to InvocationInterceptor nonetheless?

[Marcono1234]

Thanks for the feedback! I tried to address most of it and commented in the other cases, hopefully that is what you had in mind.

[Michael1993]

@Marcono1234 I don't think you need to worry about @BeforeEach or @AfterEach, since exceptions occurring there are not really part of the tested behaviour. In fact, they explicitly shouldn't be testing behaviour and so annotating them with @NotWorking (or @HasToBeFixed) is a mistake.

What do you think?

[Marcono1234]

I don't think you need to worry about @BeforeEach or @AfterEach, since exceptions occurring there are not really part of the tested behaviour.

I was thinking of the case where the cleanup in @AfterEach fails due to the bug which also causes the test to fail. But I did not intend the annotation to be used on the @AfterEach method itself.
Do you think it is worth keeping this functionality, or should it be dropped from this pull request?

The extension implements BeforeEachCallback to set up the context store, but it intentionally does not support detecting exceptions from a @BeforeEach method.

[Michael1993]

Hey @Marcono1234

Regarding this PR: we would like you to switch to InvocationInterceptor and fix the merge conflicts - if you are still available to work on this.

[Marcono1234]

As mentioned in my comment above InvocationInterceptor might not work for the @AfterEach case. In case that is not possible, do you want me to use InvocationInterceptor nonetheless even though this means the extension will have reduced functionality?

However, there is a InvocationInterceptor.interceptAfterEachMethod method. Not sure if I overlooked it back then, or if even with that method the extension would have to remember whether an exception occurred to support the @AfterEach case.

This would also include renaming the extension to @ExpectedToFail (#551 (comment)), right?

[Michael1993]

As mentioned in my comment above InvocationInterceptor might not work for the @AfterEach case. In case that is not possible, do you want me to use InvocationInterceptor nonetheless even though this means the extension will have reduced functionality?

Yes, please use InvocationInterceptor, it's much more fitting for this use-case.

However, there is a InvocationInterceptor.interceptAfterEachMethod method. Not sure if I overlooked it back then, or if even with that method the extension would have to remember whether an exception occurred to support the @AfterEach case.

Feel free to investigate this and we will discuss it with the other maintainers when it's part of the PR.

This would also include renaming the extension to @ExpectedToFail (#551 (comment)), right?

Yes, that'd be super. Thank you!

[Marcono1234]

Because this renaming and usage of InvocationInterceptor is quite different from what I originally implemented here, I am closing this and have opened a separate PR for this: #676

That PR is based on the commits of this one which hopefully makes reviewing the changes easier. I hope that is alright.