-
Notifications
You must be signed in to change notification settings - Fork 1.4k
Match anonymous tests when all of the match expressions are negative #1502
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
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.
Nice catch, thanks for the PR! I don't think matchTitleless
is necessary, see the inline comment.
lib/runner.js
Outdated
@@ -112,7 +113,7 @@ class Runner extends EventEmitter { | |||
} | |||
|
|||
if (metadata.type === 'test' && this.match.length > 0) { | |||
metadata.exclusive = title !== null && matcher([title], this.match).length === 1; | |||
metadata.exclusive = title === null ? this.matchTitleless : matcher([title], this.match).length === 1; |
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.
I think it may be sufficient to remove the title !== null
check. matcher([null],["!*oo"])
gives [null]
which passes the length === 1
check. Perhaps use matcher([title || ''], this.match)
, since the matcher expects strings.
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.
I'm okay with that simplification, but you should know that it does change behavior—that would essentially treat a titleless test the same as a test with an empty string title, so -m '*'
would truly include all tests, even the titleless ones, and there would no longer be an escape hatch to go back to the old behavior. I was trying to lean towards making this PR as conservative as possible, but I can't think of a real reason to want to distinguish between anonymous tests and empty title tests, so if you're fine with that then I'll make the change.
Yea me neither. I've scanned the discussion in #476 and I don't think we ever considered this. Generally we'd recommend against using anonymous tests, so if you do use them and need to somehow exclude them I guess you might just have to name them 😉 |
4a722e8
to
feffe0d
Compare
So shall it be! |
why not default to the file path for the name of the test if no test title is given |
@ORESoftware we already display the title as "anonymous". This PR postulates that |
@novemberborn, ready to merge? |
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.
Nice work @rhendric!
(Matching on'*'
, for example, still excludes tests with no titles, as it did prior to this commit.)The motivation here is if I have a test suite with some anonymous tests and some titled tests, and some of the titled tests are tagged with something like
[long-running]
, and I want to runava -m '!*[long-running]*'
, I don't want to exclude the anonymous tests. But with current Ava they are excluded.For what it's worth, anyone who misses the old behavior could get it back with-m '*' -m <negative match expression(s)>
.Edit: Changed so that
-m '*'
matches all tests, including anonymous. For purposes of matching, anonymous tests have empty string titles.