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

updated fail-fast plugin to take 'n' as number of failures to stop the run #616

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

gautamkhanapuri
Copy link

configuration section for this change
[failfast]
failures=2

command line
nose2 -v -F 2
nose2 -v --fail-fast 4

@sirosen
Copy link
Collaborator

sirosen commented Jul 27, 2024

Hi there, thanks for this contribution! (Sorry it's taken so long to get you feedback; it's been in my queue for a while but it's hard for me to make time for all of my various projects.)

I think fundamentally the idea here is sound -- pytest's failfast behavior also supports this kind of usage -- but I see two issues with the way that it's done here. If you can rectify these, I can see merging this change as a new feature and doing a release.

1. Breaking Interface Changes

I don't believe that changing the meaning of an existing flag is an acceptable change for nose2 at this stage of its lifecycle without very, very strong motivation.

--fail-fast is a boolean flag, and it should remain one.

I would strongly encourage you to take the following approach instead:

  • --max-fail N is a new option which controls the allowed number of failures.
  • --fail-fast is a flag which, under the default behavior, means that --max-fail 1 has been set
  • rules are established for how --max-fail and --fail-fast interact

Regarding the rules of interaction, there are a lot of possible modes, none of which should be terribly surprising, especially if they're added to the fail-fast plugin docs. For example, the flags could be treated as mutex, or --max-fail 1 could imply --fail-fast, or the combination could be treated as necessary for --max-fail to be effective. I don't feel strongly about prescribing such a behavior -- I think --max-fail implying --fail-fast but the two being compatible is nicest, but it might be harder to implement -- but it has to be well-defined.

The important issue here is the interface breakage. Because the primary user-base for nose2 are legacy applications, rather than new ones, breaking changes should be avoided whenever it is feasible.

2. New tests

Any significant functionality change like this needs new tests which exercise the feature.
I'm happy to help with this as I'm able (though, as demonstrated, it could take me quite a bit of time to get to it), but I'm not going to try to add any right now. Without the interface being sorted out, it's probably not a good use of time.


Thanks again for coming to the table with this contribution! I'm hopeful that we can resolve these and get this into an upcoming release!

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

Successfully merging this pull request may close these issues.

2 participants