Skip to content
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 some CLI testing #224

Merged
merged 4 commits into from
Apr 9, 2016
Merged

Add some CLI testing #224

merged 4 commits into from
Apr 9, 2016

Conversation

addaleax
Copy link
Member

@addaleax addaleax commented Apr 5, 2016

Add regression tests for #209 and #190. Feel very free to point out things you’d like to be changed! 😄
And btw, these tests should work on CI systems, but fail for me locally when I use the node/npm binaries that are not shipped with their tarballs (make install from the Node.js git source). istanbuljs/spawn-wrap#19 should address that, though.

@bcoe
Copy link
Member

bcoe commented Apr 5, 2016

@addaleax awesome \o/ thanks for biting the bullet and starting to write some integration tests for the CLI.

A few thoughts:

  • let's pull the cli integration tests into their own test file, perhaps called nyc-bin.js.
  • let's start displaying coverage information for bin/nyc.js.
  • let's also pull the fixtures into their own sub-folder, probably called cli.

Really appreciate your contributions \o/

@addaleax
Copy link
Member Author

addaleax commented Apr 5, 2016

let's pull the cli integration tests into their own test file, perhaps called nyc-bin.js.

Sounds like a good idea. Two things:

  1. This would require re-writing build-tests.js to handle both files, or make it take an argument indicating which source file to split up, right?
  2. There are already some tests which spawn the nyc cli – Should these be moved, too?

@bcoe
Copy link
Member

bcoe commented Apr 5, 2016

@addaleax correct me if I'm wrong, but I don't think the build step for source-map-cache.js is as complex as the nyc-test.js build process? I think we can probably copy the approach used for source-map-cache.js.

I would say: if the tests are specifically exercising aspects of the cli: how it parses arguments, its various commands, its exit codes; let's pull them into this new file.

The build step of the tests is designed to test the nitty-gritty functionality if index.js in a few different modes of operation, cached, uncached, etc.

@addaleax
Copy link
Member Author

addaleax commented Apr 5, 2016

@bcoe No, you’re right, that’s why I’m asking :-) I’ll try that then, shouldn’t take too long.

@addaleax
Copy link
Member Author

addaleax commented Apr 5, 2016

Btw, are you fine with the added symlinks? I don’t quite remember how current git implementations on Windows handle them (… is that why AppVeyor won’t start?) Removed the symlinks while rebasing

@addaleax
Copy link
Member Author

addaleax commented Apr 5, 2016

@bcoe Since all CI tests run on Node.js >= 0.12, is using child_process.spawnSync okay? That would definitely make the tests more concise.

@jamestalmage
Copy link
Member

I don't think the build step for source-map-cache.js is as complex as the nyc-test.js build process? I think we can probably copy the approach used for source-map-cache.js.

Correct. nyc-test.js is split into multiple files so you get a clean process for each test. This ended up being necessary because of the way nyc mucks with the currently running process (and because it doesn't behave well when you invoke it multiple times on the same process).

You probably still need a build step to create bin/nyc.covered (same as how source-map-cache.covered.js is created).

You should do something similar to this to determine if the covered file exists or not.


@addaleax If the above stuff regarding coverage is confusing, feel free to just focus on writing some good tests and I can add coverage in another PR when you are done.

@addaleax
Copy link
Member Author

addaleax commented Apr 5, 2016

@jamestalmage It’s not confusing by itself – like, I know the purpose and how it works in theory – but you obviously have the advantage of practical experience with the necessary steps. So, if you have the time, feel free to do that 👍

@addaleax addaleax force-pushed the cli-testing branch 2 times, most recently from 6a0dfc2 to 1e5a17c Compare April 7, 2016 01:43
@addaleax
Copy link
Member Author

addaleax commented Apr 7, 2016

@bcoe The last commit adds the test cases I talked about… Travis should become green when including the spawn-wrap PR (Edit: it does 🎉)

Check that `--check-coverage` sets the exit code correctly for
sufficient/insufficient line coverage, and make sure that
that does not override a failure-indicating return code from
the test command.
Adds tests for checking that `nyc npm test` works,
one for when the npm `test` script directly refers to the
JS script and one for when npm runs nested lifecycle scripts.
This is a regression test for istanbuljs#190.
Move the recently added test which do not touch `index.js` code
directly into `nyc-bin.js` and their corresponding fixtures to
`test/fixtures/cli`.
This also belongs to istanbuljs#190, but the new tests include actual
fake “npm” executables with both absolute shebangs and the
twisted shebang that comes with current Node.js binary distribution
tarballs.
@bcoe
Copy link
Member

bcoe commented Apr 8, 2016

@addaleax looks like you might have already, but if you rebase these tests should now start passing.

@addaleax
Copy link
Member Author

addaleax commented Apr 9, 2016

@bcoe yup, just did after you merged #226 😄 but Travis is green here too, so that should be fine… is there anything else that should definitely go into this PR?

@bcoe
Copy link
Member

bcoe commented Apr 9, 2016

@addaleax this rocks and you rock.

@bcoe bcoe merged commit 3403ca1 into istanbuljs:master Apr 9, 2016
@addaleax addaleax deleted the cli-testing branch April 11, 2016 00:44
@bcoe bcoe mentioned this pull request Apr 11, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants