-
-
Notifications
You must be signed in to change notification settings - Fork 6.5k
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
Allow array of regexp strings for testRegex #7209
Changes from 5 commits
c2c23f0
f08508d
c4f189b
eb8355f
e25d89c
3b66dc8
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -29,7 +29,13 @@ describe('should_instrument', () => { | |
|
||
it('when testRegex provided and file is not a test file', () => { | ||
testShouldInstrument('source_file.js', defaultOptions, { | ||
testRegex: '.*\\.(test)\\.(js)$', | ||
testRegex: [/.*\.(test)\\.(js)$'/], | ||
}); | ||
}); | ||
|
||
it('when more than one testRegex is provided and filename is not a test file', () => { | ||
testShouldInstrument('source_file.js', defaultOptions, { | ||
testRegex: [/.*\_(test)\.(js)$/, /.*\.(test)\.(js)$/, /never/], | ||
}); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I see that we only test happy path in this test file. It would be great to add at least two smoke tests (1 for testRegex, 1 for testMatch) when the file should not be instrumented (e.g. add 4th There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There are already two I do see that the description of the spec was incorrect, I have fixed that, as well as a couple other existing ones that were also incorrect. Maybe this was throwing you off - either way if there's still some additional coverage you'd like to see let me know. I have also added a test of error condition to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh my, I totally missed second |
||
}); | ||
|
||
|
@@ -88,7 +94,7 @@ describe('should_instrument', () => { | |
testShouldInstrument('do/collect/sum.coverage.test.js', defaultOptions, { | ||
forceCoverageMatch: ['**/*.(coverage).(test).js'], | ||
rootDir: '/', | ||
testRegex: '.*\\.(test)\\.(js)$', | ||
testRegex: ['.*\\.(test)\\.(js)$'], | ||
}); | ||
}); | ||
}); | ||
|
@@ -115,7 +121,13 @@ describe('should_instrument', () => { | |
|
||
it('when testRegex provided and filename is a test file', () => { | ||
testShouldInstrument(defaultFilename, defaultOptions, { | ||
testRegex: '.*\\.(test)\\.(js)$', | ||
testRegex: [/.*\.(test)\.(js)$/], | ||
}); | ||
}); | ||
|
||
it('when more than one testRegex is provided and filename matches one of the patterns', () => { | ||
testShouldInstrument(defaultFilename, defaultOptions, { | ||
testRegex: [/.*\_(test)\.(js)$/, /.*\.(test)\.(js)$/, /never/], | ||
}); | ||
}); | ||
|
||
|
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.
also, what do you think about naming this just
multiple
for simplicity? I'm good with both, so pick whatever you like :)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 don't feel strongly, but I guess I'd lean towards leaving it more verbose to provide clear context, since
Mutliple
could potentially mean more than one thing.