-
-
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
Add projectConfig argument to shouldRunTestSuite hook #6350
Conversation
@@ -61,11 +61,11 @@ class JestHooks { | |||
this._listeners.onTestRunComplete.forEach(listener => | |||
listener(results), | |||
), | |||
shouldRunTestSuite: async testPath => | |||
shouldRunTestSuite: async (testPath, config) => |
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.
Another option for this would be to change the signature to ({ testPath, config }) => {}
, which might be better for future API changes, given that adding keys to that object would not be a breaking change.
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.
Although this would mean an initial breaking change, which should probably only affect jest-watch-typeahead
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 hope people will understand it's still getting in shape ;)
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.
We can call it out in the blog post as still somewhat experimental?
packages/jest-cli/src/run_jest.js
Outdated
@@ -58,7 +58,9 @@ const getTestPaths = async ( | |||
} | |||
|
|||
const shouldTestArray = await Promise.all( | |||
data.tests.map(test => jestHooks.shouldRunTestSuite(test.path)), | |||
data.tests.map(test => | |||
jestHooks.shouldRunTestSuite(test.path, test.context.config), |
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.
How about passing a stripped information about test, e.g. ({path, duration, config})
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.
+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.
Yeah that seems like the best idea
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.
Although duration is not always present, right?
packages/jest-cli/src/run_jest.js
Outdated
@@ -58,7 +58,9 @@ const getTestPaths = async ( | |||
} | |||
|
|||
const shouldTestArray = await Promise.all( | |||
data.tests.map(test => jestHooks.shouldRunTestSuite(test.path)), | |||
data.tests.map(test => | |||
jestHooks.shouldRunTestSuite(test.path, test.context.config), |
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.
+1
types/JestHooks.js
Outdated
shouldRunTestSuite: (testPath: string) => Promise<boolean>, | ||
shouldRunTestSuite: ( | ||
testPath: string, | ||
config: ProjectConfig, |
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.
Yeah, from the discussion above I could see the type here being TestSuiteConfig
which is some subset of Test
jestHooks.shouldRunTestSuite({ | ||
config: test.context.config, | ||
duration: test.duration, | ||
testPath: test.path, |
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.
(testPath
vs path
) and (config
vs projectConfig
):
I like testPath
and config
better, but I'm fine with anything
Codecov Report
@@ Coverage Diff @@
## master #6350 +/- ##
=======================================
Coverage 63.63% 63.63%
=======================================
Files 226 226
Lines 8648 8648
Branches 4 4
=======================================
Hits 5503 5503
Misses 3144 3144
Partials 1 1
Continue to review full report at Codecov.
|
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.
Changelog? :D
Yes 🤦♂️ , I’ll add it when I wake up tomorrow morning, because I’m not in my computer right now |
Haha, timezones are weird. I'm on the bus on my way to work |
I'll let you send a new PR with the changelog entry pls! #movefast |
This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
Summary
Currently, the
shouldRunTestSuite
hook only takes thetestPath
. I did that intentionally when designing plugins to keep the API surface as small as possible, but it seems that getting the ProjectConfig, along with thetestPath
might be useful.Another idea was to pass the whole
test: Test
object, which seems really nice but I'm worried that it will leak out a lot of implementation details, for examplecontext.moduleMap
andcontext.hasteFS
... The only other option inside test that would be interesting to get, isduration
.This should allow us to do something like a
project selection plugin
.