-
Notifications
You must be signed in to change notification settings - Fork 783
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
Core: Connected to #1434: run all only
tests
#1436
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for implementing this approach as well! @qunitjs/core any of you have thoughts on this approach over #1435?
Thanks @trentmwillis, let me know which approach is preferred and I'll complete this by adding docs :-). |
only
tests.only
tests.
I'm in favor of this "running all only's" behavior. The new logic is clear ("run only these!"), where the current algorithm was a little fuzzy on which on won. The "run some" approach with a different API has a lot of behavioral overlap (two ways of doing the same thing). It helps close this gap, but introduces more fuzziness with it. I don't think this change should be cause for a major semver bump, just a minor. |
I'm +1 on improving As I understand it, its design was limited to one as a way to to avoid mistakes, not per se because it would be undesirable to support multiple. I do note that if we change this behaviour, the docs should be updated as well. In particular, at https://api.qunitjs.com/QUnit/only it currently says:
|
Thanks @Krinkle, as we seem to be leaning towards changing |
only
tests.only
tests
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall, this looks great! A few small suggestions for the documentation and one question about the actual implementation: does this handle nested modules properly? For example, if you had:
module.only('One', () => {});
module.only('Two', () => {
module('A', () => {});
});
Would module A
run?
I'm pretty sure the following would work:
module.only('One', () => {
module('A', () => {});
});
But I think if a module.only
is followed by another module.only
with the second one having nested modules, then the nested modules won't run (even though I think a user would expect them to).
Thanks @trentmwillis, I agree that modules nested inside a |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me! Thank you! I'll probably release a new version this weekend.
Thanks @trentmwillis. Looking forward to it :-). |
Released in v2.10.0! |
Thanks! |
WIP alternate implementation of #1434: modifying the way
only
behaves instead of adding a new method.TODO: