Provide annotation that a test currently fails by given circumstances

[Bukama]

Creating an issue as base for the PR for a @NotWorking annotation. Text comes from the PR by @Marcono1234

---

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.

It is therefore recommended to prefer using @NotWorking over @Disabled
in most cases; exceptions for this are for example 'flaky' tests.

Important: This annotation is not intended as a way to mark test methods
which intentionally cause exceptions.
Such test methods should use JUnit's assertThrows or similar means to explicitly
test for a specific exception class being thrown by a specific action.

[nipafx]

I really like this idea, it just makes sense to have the option to bake the assumption that the test fails into the enable/disable logic. Straight-up winner on that front.

But I'm wondering, whether it isn't still too close to @Disabled. All the requirements that exist for (disable on OS, on JDK version, until a date, for a specific parameter, etc.) could apply here as well and reimplementing all of that functionality for a new variant of that enable/disable behavior seems so wasteful. The question is whether there's a way around that.

Is there a way to implement this as a kind of "modifier" on @Dsiabled, e.g.:

@NotWorking
@DisabledOnOs(MAC)
void test() {
	// ...
}

Then @NotWorking would have to detect @DisabledOnOs's decision and, if it wants to disable, override that and wrap it into the behavior described above. It's very unclear to me whether that can work given that some of the @Disabled... annotations come from Jupiter Proper.

One weird trick that could work would be:

• copy all Jupiter @Disabled... annotations and extensions to Pioneer (maybe under a different class name?)
• edit them and all of our annotations to interact with @NotWorking as described above

That's... probably not a good idea? 🤷🏾

[Marcono1234]

I only had a brief look at how @Disabled is implemented by JUnit, but to me it appears ideally the @NotWorking code would hook into org.junit.jupiter.engine.execution.ConditionEvaluator.evaluate. However, I am currently not seeing a way how this could be done easily.

[Bukama]

Just found: #468 (Abort tests if URL is unreachable) which also goes into this direction

[nipafx]

Thanks for looking into it, @Marcono1234. This is the relevant code:

/**
 * Evaluate all {@link ExecutionCondition} extensions registered for the
 * supplied {@link ExtensionContext}.
 *
 * @param context the current {@code ExtensionContext}
 * @return the first <em>disabled</em> {@code ConditionEvaluationResult},
 * or a default <em>enabled</em> {@code ConditionEvaluationResult} if no
 * disabled conditions are encountered
 */
public ConditionEvaluationResult evaluate(ExtensionRegistry extensionRegistry, JupiterConfiguration configuration, ExtensionContext context) {
	// @formatter:off
	return extensionRegistry.stream(ExecutionCondition.class)
			.filter(configuration.getExecutionConditionFilter())
			.map(condition -> evaluate(condition, context))
			.filter(ConditionEvaluationResult::isDisabled)
			.findFirst()
			.orElse(ENABLED);
	// @formatter:on
}

It makes obvious that there's no way for us, as one of many ExecutionConditions, to interact with another, and so there's nothing fancy we can do here except we go with the "one weird trick" I described above, but that's a really extreme step that I'd like to avoid.

So we're stuck with @NotWorking (although I don't like the name too much, but that's a different conversation) having to reimplement all of that "on OS", "on Java version", etc. if the need arises in the future, i.e. not in the first release.

[nipafx]

Two thoughts...

Name

We need to see whether there's a better name: @NotWorking describes expectations about the test ("it doesn't work"), but not about the test execution ("will it run?" "if so, what happens if it fails/succeeds?"). I'm not sure we can express all of that in a name, but hopefully more than we do now.

I have something like @ExpectFailing in mind, but am not very happy with that idea either and hope for others. :)

Configuration

The extension I introduce in #505 also needs to act on a test's thrown exceptions. It allows configuration of which specific exceptions (e.g. @DisableIfTestFails(with = IOException.class) - by default all Throwables) are supposed to be processed and whether assertion errors can be ignored (e.g. @DisableIfTestFails(onAssertion = false) - by default assertions are included). I think the same logic could be applied here. This can be a follow-up issue.

I also noticed how the extension handles assumptions and think that makes a lot of sense. Yet, it could be made configurable as well. Come to think of, does #505 correctly handle failing assumption?

[Marcono1234]

I have something like @ExpectFailing in mind

The reason why I wanted to avoid such name containing "expect" is that it makes it sound like the developer intentionally wants the exception. While actually the developer only acknowledges that there is currently a bug which prevents the test from succeeding. @ExpectFailing could be misunderstood as an alternative to assertThrows which this feature is not.

I am afraid that when you want to include the evaluation of the execution ("if so, what happens if it fails/succeeds?") in the name, you always risk making it sound like the extension is an alternative for assertThrows.

[nipafx]

I thought about the name a bit more. We basically want to communicate three things:

1. the test is executed
2. a failing result isn't reported
3. a succeeding test is marked failed

I talked to a few people at DevoxxUK just now to get their input. Two proposals:

• @Silenced - This would be a new concept similar to "disabled". It contains the notion of the test being executed and a (failing) result not being reported. It doesn't imply that a successful run will fail the build, though.
• @Fails, @FailsTemporarily, or similar - Clearly describes the expectation (better than @NotWorking in my opinion), but doesn't speak directly to either of the three points. It immediately begs the question how that's handled, though.

Thoughts?

[Bukama]

I like the wording with @Silenced. Maybe we can use that in a way.

[Michael1993]

I don't like @Silenced - to me that communicates that the test is silenced regardless of outcome, which misses point 3 from the list (a succeeding test is marked failed).

I also don't like @Fails because it seems to communicate that the test itself fails, which misses point 2 from the list (a failing result isn't reported).

Honestly, I like your original idea, @ExpectFailing or something similar (e.g.: @ExpectedToFail).
It fits all 3 points:

1. since we have an expectation, it communicates that the test is executed (to verify those expectations)
2. since we expect failure, test failure doesn't have to be reported
3. since we expect failure, test success has to be reported

[nipafx]

To keep this issue and the related PR from stalling, I'm just gonna make the call and say the extension should be named @ExpectedToFail.

@Marcono1234 Are you ok with that? Or at least ok enough to finish the PR? 😉 If not, that's ok, too, just let us know and we'll finish it.

[Marcono1234]

To keep this issue and the related PR from stalling, I'm just gonna make the call and say the extension should be named @ExpectedToFail.

@Marcono1234 Are you ok with that? Or at least ok enough to finish the PR? 😉 If not, that's ok, too, just let us know and we'll finish it.

I still think that "expected", especially in the context of unit tests, might erroneously give the impression that it is desired that the test fails, while it is actually merely "acknowledged", see also my comment #551 (comment) above.

However, I am not strongly opposed to this, so if you want to call it @ExpectedToFail, then that is ok for me. What else (except for the renaming) has to be done for the pull request? I think the question how the extension should behave regarding @BeforeEach and @AfterEach is still open (see latest comment on the PR).

Also, if you think it is easier, feel free to work on the pull request yourself (or use it as base for your changes); but please let me know in that case beforehand.

[nipafx]

I still think that "expected", especially in the context of unit tests, might erroneously give the impression that it is desired that the test fails, while it is actually merely "acknowledged", see also my comment #551 (comment) above.

I understand, but I still think it's the best option.

What else (except for the renaming) has to be done for the pull request?

I'll take another look and let you know over there.

Also, if you think it is easier, feel free to work on the pull request yourself (or use it as base for your changes); but please let me know in that case beforehand.

I don't think I'll do that, but good to know there's the option (you never know when boredom presents itself 😉). Will let you know if that happens. 👍🏾

[Marcono1234]

I just came across this project implementing a similar idea: https://github.com/MartinWitt/GithubIssue

Probably not that relevant for this implementation, but maybe interesting nonetheless that someone else had a similar idea, and how they implemented it.

Edit: Not sure if that extension works properly though; looks like TestWatcher used by that extension is not intended to throw exceptions, see relevant JUnit code.