add label option "BeforeAndAfter"

[johannes-schmitt]

fixes #628

[dnfclas]

All CLA requirements met.

[CharliePoole]

I'm holding off on code review till more team members weigh in on the use of All versus BeforeAndAfter versus other options. See issue #628 for discussion.

[rprouse]

I don't like All because it is ambiguous. If anything I think we should deprecate it. To me, BeforeAndAfter is clear what it means.

[CharliePoole]

@rprouse IIRC we "deprecated" it in our discussion but didn't have any way to express that deprecation either at the command line or as an engine option, which is a string value. If anyone can think of a way to do that, which will be visible to users, it might be worth trying.

[CharliePoole]

OTOH one could argue that if All showed all possible labels for all tests, it would no longer be ambiguous.

[rprouse]

@CharliePoole you are right. We could just deprecate it by no longer documenting it and/or we could deprecate it by printing a warning note on the console when it is used. To be honest, I think we should have taken a stronger stand when we introduced the Before and On labels together. I personally find those labels confusing and always have to look up which one does which.

My problem with All in this case is back to that confusion. In this case, I think we want it to mean On | After so that a label is printed as soon as a test starts and another as soon as a test ends. Other people might want it to mean Before | After. Which is correct?

I don't have a strong opinion on this, I just don't like making an already confusing feature potentially more so 😉

[CharliePoole]

@rprouse I felt there was some confusion about how we got where we are so I posted s mini-history of the feature on the issue. You can see that On and All preceded Before and After.

Of course, history explains why we're here but doesn't tell us what to do next.

Personally, I think that developing an API incrementally means you will occasionally need to break it. I wish we had dropped All when we added the synonym Before, but we didn't.

[CharliePoole]

@rprouse Just reread your last paragraph. After already includes On. So does Before and All. I was suggesting we redefine All as Before plus After unless we ate willing to get rid of it entirely.

I don't think it helps to treat this as a flags enum because the option controls two subtly different things: labelling of tests and labelling of test output.

[mikkelbu]

@rprouse IIRC we "deprecated" it in our discussion but didn't have any way to express that deprecation either at the command line or as an engine option, which is a string value. If anyone can think of a way to do that, which will be visible to users, it might be worth trying.

No so long ago Joseph added a deprecation warning for --params in the beginning of the console output (and in the help from console), see e.g.

nunit-console/src/NUnitConsole/nunit3-console/CommandLineOptions.cs

Lines 288 to 294 in ec66e00

	this.Add("params|p=", "Deprecated and will be removed in a future release. Please use --testparam instead.",
	v =>
	{
	const string deprecationWarning = "--params is deprecated and will be removed in a future release. Please use --testparam instead.";
	if (!WarningMessages.Contains(deprecationWarning))
	WarningMessages.Add(deprecationWarning);

[CharliePoole]

@mikkelbu Thanks, I'd forgotten about that.Same thing would work here.

[ChrisMaddock]

Thanks for doing this @johannes-schmitt - looks good.

@nunit/engine-team I agree with the general sentiment that this API is getting too confusing. I'd like to propose the changes below for the overall API. I do think they are out of scope for this PR however - if we agree on this (or similar) - I think we should merge this as is, and make the later changes as a separate issue.

---

My thoughts on the existing API (that I think we're in agreement on) is that On and All are now ambiguous and not futureproof. I suggest the following changes, as which would allow us to clean up the current API, and better make further changes in future.

Option	Change
Off	No change
After	No change
Before	No change
BeforeAndAfter	Show labels at start and end of each test case - as per this PR.
OutputOnly	New option, replaces 'on', but with a less ambiguous name.
On	Deprecate - Remove doc and print warning, as per --params
All	Deprecate - Remove doc and print warning, as per --params

[CharliePoole]

@ChrisMaddock Makes sense to me.

[jnm2]

@ChrisMaddock I like that.

If now's the time to change stuff, what use is the Off option? If there ever is output, I'd be inclined to say the user doesn't get to decide not to show the test name that is responsible for it.

[CharliePoole]

@jnm2 Since Off is the default, eliminating it would be a pretty major change, bigger than just changing the spelling of an option.

[jnm2]

Okay. I didn't realize that.

[rprouse]

Excellent proposal @ChrisMaddock 👍