Skip to content
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

v4: Remove deprecated --labels options #891

Closed
ChrisMaddock opened this issue Jan 30, 2021 · 7 comments · Fixed by #1144
Closed

v4: Remove deprecated --labels options #891

ChrisMaddock opened this issue Jan 30, 2021 · 7 comments · Fixed by #1144
Assignees
Milestone

Comments

@ChrisMaddock
Copy link
Member

From memory, I think it's just On which is now redundant.

@CharliePoole
Copy link
Contributor

I always liked "On" as a clean way to specify showing labels in whatever way we consider the default. Up to now that has been "Show a label whenever there is output unless preceded by another line of output from the same test" but we might change that from release to release.

@CharliePoole
Copy link
Contributor

IIRC this is a console option that has nothing to do with the engine. That leads me to wonder if we should split versioning for the console and engine as a part of 4.0... do we have that already?

@ChrisMaddock
Copy link
Member Author

Last we talked about that - we discussed doing it in 4.1, as it was non-breaking. It was something I wanted to think about a bit more...is it breaking?

I feel like console 4.0 definitely needs to happen at the same time as engine 4.0, if we're going to remove options like process and domain. Unless we're going to have the console reference the 3.11 engine, but I'd rather both moved at roughly the same time, till we can split them properly.

@CharliePoole
Copy link
Contributor

I think there are two points of view here...

  1. It's breaking because users can no longer type the option you remove and any scripts that use it won't work.
  2. It's non-breaking because the error will be obvious and it's a small change to make.

If we followed the spirit of my VERSIONING writeup for the GUI, this would be a minor change... we are not removing a capability but changing how it is accessed. Of course, in the case of the GUI, there is a writeup to refer to.

Since we know that various CI programs launch the console runner, it's probably safest to consider it breaking.

I'm happy with 4.0 for the whole thing all at once... I was just wondering.

@CharliePoole
Copy link
Contributor

@nunit/engine-team I removed "Idea" because we have to do something here! I added "Needs Discussion" because we need to decide a few things.

The two deprecated options are --labels:On and --labels:All. Currently, both work but you get a warning message, telling you to use OnOutputOnly for On and Before for All.

I'll start with All / Before... I think that one is a no-brainer. It wasn't confusing when we only had Off, On and All. It meant "label alltests" rather than just those with output. But once we had Before and After, it became confusing... i.e. it could mean "both before and after." I think we should go ahead and remove All as an option value.

I'm not in agreement with removing "On". Here is what I'd like to support:

Off
Don't display any labels

On
Display labels using our internal default setting, which we might change in future. Currently, it's OnOutput.

OnOutput
Display labels before each test producing output. If tests run in parallel, repeat the labels as needed when there is a change in which test is producing output.

Before
Display labels before each test, whether it produces output or not. If there is output, handle parallel tests just as for OnOutput

After
Display labels after each test, including the test result.

BeforeAndAfter
Display labels both before and after each test.

Breaking Changes

If we do the above, the following are breaking changes from 3.15.

  • Change OnOutputOnly to OnOutput
  • Remove All

By keeping On and defining it to mean "Use our internal default" we enable changing that default in the future without a major revision. That is, the user may either use On or OnOutput depending on how important it is to them that the meaning of the option should not change.

Comments?

@CharliePoole
Copy link
Contributor

Absent comments, I'm going with the above.

@jnm2
Copy link
Collaborator

jnm2 commented Feb 18, 2022

This makes sense to me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants