Junit5: Support custom reason in @Disabled* / @Enabled* variants

[filiphr]

I would like to be able to write the reason why a test is disabled or enabled on a particular OS, JRE, etc. Currently the text is fixed in the different ExecutionCondition(s).

I would propose that a new default String reason() "" is added to the variants and that reason is suffixed to the current default text for each condition.

Deliverables

TBD

[sbrannen]

Tentatively slated for 5.5 M1 for _team discussion_

[sbrannen]

Tentatively slated for 5.7 M1 for _team discussion_

[marcphilipp]

Team decision: Let's add the optional reason attribute to all @Disabled* / @Enabled* variants.

[juliette-derancourt]

With the current default reasons, we differentiate the enabled/disabled cases (for instance with reasons like Enabled on operating system: Linux and Disabled on operating system: MacOS).
Should we do the same here and ask the user to provide two reasons (one to use if the test ends up being disabled, and another if it doesn't)? Or should we assume that they are gonna write a reason that will "make sense" in both cases (like This test can only run on Linux because... instead of Disabled on Linux because...)?
Alternatively, we could also use the provided reason when the test is disabled, and keep the default one when it's not (since I think this reason is never really printed to the user?)

Tbh, I think that one reason is enough but I thought it was worth bringing the question up.
@junit-team/junit-lambda What do you think?

[marcphilipp]

Thanks for raising the question! I think we also need to decide whether the generated reason should stay in place and the additional reasons would only be added, e.g. using a template like <auto-generated> because <explicit reason>.

[filiphr]

If I can propose, then I would propose to keep the generated reason and add the explicit reason written by the user.

My main idea when raising this issue was to enrich the auto generated reason with an explanation why we are disabling / enabling the particular test.

[sbrannen]

I would propose that a new default String reason() "" is added to the variants and that reason is suffixed to the current default text for each condition.

Yes, that's exactly what I would do: append the custom message to the generated message -- for example, separated by ==> like we do with all of our failed assertion message.

assertEquals(42, 99, "42 != 99");

org.opentest4j.AssertionFailedError: 42 != 99 ==> expected: <42> but was: <99>

The difference is that we prepend the custom message to failed assertion messages; whereas, I agree that appending a custom reason to execution conditions seems to be nicer.

So, given @DisabledOnOs(value=WINDOWS, reason="incompatible filesystem"), the combined reason would be something like:

Disabled on operating system: Microsoft Windows ==> incompatible filesystem

[sbrannen]

ask the user to provide two reasons (one to use if the test ends up being disabled, and another if it doesn't)?

That's a tough one. I'm still thinking about the options.

Or should we assume that they are gonna write a reason that will "make sense" in both cases (like This test can only run on Linux because... instead of Disabled on Linux because...)?

No, I don't think we can assume that. People often don't like to be overly verbose. For example, I just wrote an example with "incompatible filesystem" for a disabled reason.

That's because it seemed logical to me. I added the "reason" that it is disabled on Windows. If you simply _read_ the annotation, it makes sense.

So if we introduce a single new attribute named reason, I assume most people will think it's the "enabled reason" in an @Enabled* annotation and the "disabled reason" in a @Disabled* annotation.

But your line of questioning is good! What reason would we use for an @Enabled* annotation with a custom reason _if_ the result is that it's disabled?

If we encounter the following on Linux...

@EnabledOnOs(value=WINDOWS, reason="compatible filesystem")

... then the following doesn't make sense.

Disabled on operating system: Linux ==> compatible filesystem

It's actually disabled because it is not a "compatible filesystem".

Alternatively, we could also use the provided reason when the test is disabled, and keep the default one when it's not (since I think this reason is never really printed to the user?)

If we do that, I think we'll have to rename reason to disabledReason due to the oddity I pointed about above with the _disabled_ @Enabled* declaration. That would result in:

@EnabledOnOs(value=WINDOWS, disabledReason="incompatible filesystem")

... which would lead to the following on Linux:

Disabled on operating system: Linux ==> incompatible filesystem

... which now makes sense.

So, that was a bit of brainstorming out-loud, and I think I now agree with you: just support one custom reason. But... we should probably name it disabledReason and explain the semantics for when it is appended to the standard reason and when it is omitted.

[juliette-derancourt]

I would propose that a new default String reason() "" is added to the variants and that reason is suffixed to the current default text for each condition.

I now realise I overlooked the second part of that sentence... Yes, I also think it makes sense to combine both default and custom reasons.

As for the difference between enabled/disabled reasons, thank you @sbrannen for clarifying the issue I was refering to! 😄
I agree, if we only support the "disabled" reason we should name it accordingly + document it properly.