Add tests for command "run" #821
Conversation
|
|
||
| test.concurrent('run should run binary', (): Promise<void> => { | ||
| return runRun({}, ['echo'], 'run-should-run-binary'); | ||
| }); |
There was a problem hiding this comment.
Oh, I see what this is doing! If you do this, and add specs for the same thing but with spaces in the fixture directory name, that should have you covering the issue #633 intended to fix completely! 😃
There was a problem hiding this comment.
Yup, the rest should be just copy&paste. I just want to be sure that I am on the right path. Currently only "add", "install" and "self-update" commands are tested so there isn't much to use as an example.
There was a problem hiding this comment.
Come to think of it, it could be reasonable to mock out child_process (used all the way over here) for these purposes, but I guess it depends if you want to actually integration test with the shell.
AlicanC
force-pushed
the
alicanc-run-tests
branch
from
1adbea1 to
d5ca2fe
Compare
|
Updated PR and original post. |
AlicanC
force-pushed
the
alicanc-run-tests
branch
2 times, most recently
from
95d34eb to
2d1cc8b
Compare
| } | ||
|
|
||
| test.concurrent('run should run script', (): Promise<void> => { | ||
| return runRun({}, ['some-script'], 'run-should-run-script'); |
There was a problem hiding this comment.
great job!
Usually tests should have something to expect, not just run and expect that it does not crash.
Can you add expectations that verify that the command has been executed?
There was a problem hiding this comment.
I couldn't find a way to do that, but instead I've added expectations for calling of executeLifecycleScript().
I think these tests should only verify that run() can call executeLifecycleScript() properly, then another test can verify if executeLifecycleScript() can do its job properly.
AlicanC
force-pushed
the
alicanc-run-tests
branch
from
d514eb7 to
54cafbe
Compare
|
Thanks @AlicanC! |
This reverts commit 782fd84.
|
Hey @AlicanC, the CI failed with errors https://circleci.com/gh/yarnpkg/yarn/1806. |
Summary
In #809, suggested tests were these (twice, in straight and white-spaced paths):
yarn run test -- --helpyarn run test-helpyarn run jest -- --helpIn this PR I test the internal run() command and (AFAIK) it is not responsible converting
yarn run test -- --helptotest --helpso the part where-- --argbecomes--argshould not be covered.What I cover is:
package.json.node_modules/.bin/.node_modules/.bin/with a white-spaced path.node_modules/.bin/with a single arg.1, 2 and 4 was working on release. #663 fixed 3, but broke 4 and with #809 all tests pass.
There are more to add, but I want to be sure if I'm doing this right. I'd like to get this merged (if it is right) and add others in another PR when I have more time.
Please comment.
Test plan
(What should a test plan be for a test?)