-
-
Notifications
You must be signed in to change notification settings - Fork 5.6k
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
Support specifying minimum Node version a test requires #5765
Conversation
@buunguyen, thanks for your PR! By analyzing the history of the files in this pull request, we identified @hzoo, @zertosh and @existentialism to be potential reviewers. |
looks good, yeah we wouldn't need a test other than the usage of it like we needed 🙂 in the issue originally |
Codecov Report
@@ Coverage Diff @@
## 7.0 #5765 +/- ##
==========================================
- Coverage 84.65% 84.58% -0.07%
==========================================
Files 282 282
Lines 9854 9862 +8
Branches 2766 2769 +3
==========================================
Hits 8342 8342
- Misses 998 1005 +7
- Partials 514 515 +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.
anyone else have thoughts on the name? fine to me
} | ||
|
||
// Delete to avoid option validation error | ||
delete taskOpts.minNodeVersion; |
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.
Why do we have to delete?
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.
If not, the test will throw Unknown option: base.minNodeVersion.
I suppose we can update OptionManager
to support this option, but given it's only internally used, I'm not sure if we should do that.
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.
Since it uses semver, why not just {nodeVersion: ">4"}?
That's an option too. Because here we explicitly want minimum version, I though qualifiers like '>', '<' or even range is not necessary. Specifying a single minimum version seems to be simplest for the purpose. |
Add an option
minNodeVersion
so that test case can bail out if the current Node version isn't >= the expected version. This is to avoid having to check version in each test and do the eval hack (see #5752).I can't figure out a good way to write automated test for this. I tested by adding
options.json
manually, changingminNodeVersion
and observing behavior. I don't think an automated test is very important for this, but if you disagree, please let me know (and any idea to write this test is very welcome).Also, there's a choice of making this change here or in transform-fixture-test-runner. I decided to bail out as early as possible, so here makes more sense.