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.
|
hzoo
left a comment
There was a problem hiding this comment.
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.
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.
xtuc
left a comment
There was a problem hiding this comment.
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
minNodeVersionso 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.jsonmanually, changingminNodeVersionand 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.