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

feat: Allow nyc instrument to instrument code in place #1149

Merged
merged 3 commits into from
Aug 1, 2019

Conversation

AndrewFinlay
Copy link
Contributor

@AndrewFinlay AndrewFinlay commented Jul 21, 2019

This PR addresses #1135

This change adds the --in-place switch to nyc instrument
If --in-place is specified, the output directory will be set to equal the input directory for the instrument command.
This has the effect of replacing any file in the input directory that should be instrumented, with its instrumented counterpart.
The command will throw an error if the --delete option is specified.
The only way to instrument in place is with the --in-place switch, setting the input and output directories to be the same will not work
If --in-place is set the instrument command ignores any output directory specified with the command
If --in-place is set the instrument command will disable the --complete-copy switch as it is unnecessary.

I've made a few small code improvements to the instrument command spec.
I've also added tests to the old integration tests folder, I figured I could put tests here for the time being before they get moved to the new tap format (by me or someone else).

This change adds the `--in-place` switch to nyc instrument
If `--in-place` is specified, the output directory will be set to equal the input directory for the instrument command.
This has the effect of replacing any file in the input directory that should be instrumented, with its instrumented counterpart.
The command will throw an error if the --delete option is specified.
The only way to instrument in place is with the --in-place switch, setting the input and output directories to be the same will not work
If `--in-place` is set the instrument command ignores any output directory specified with the command
If `--in-place` is set the instrument command will disable the `--complete-copy` switch as it is unnecessary.

I've made a few small code improvements to the instrument command spec.
I've also added tests to the old integration tests folder, I figured I could put tests here for the time being before they get moved to the new tap format.
@AndrewFinlay
Copy link
Contributor Author

Just a quick note that the I'm aware that this breaks the test snapshot and you probably don't want additions to the old integration test file. Since I'm a little low on time at the moment I figured that it'd be good to get this out there and it'd be quicker to make tests for the old integration set that I'm familiar with. Anyway let me know what needs to be done from here, thanks

Copy link
Member

@coreyfarrell coreyfarrell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a quick note that the I'm aware that this breaks the test snapshot

Yes please run npm run snap, commit the changes to tap-snapshots.

you probably don't want additions to the old integration test file.

Don't worry about this, the tests you've added really are not appropriate for snapshot testing. A couple extra tests using the mocha emulation is not a big issue.

Since I'm a little low on time at the moment I figured that it'd be good to get this out there and it'd be quicker to make tests for the old integration set that I'm familiar with. Anyway let me know what needs to be done from here, thanks

thanks for the patch, just a couple of minor issues I mentioned.

docs/instrument.md Outdated Show resolved Hide resolved
lib/commands/instrument.js Show resolved Hide resolved
test/nyc-integration-old.js Outdated Show resolved Hide resolved
test/nyc-integration-old.js Outdated Show resolved Hide resolved
@coreyfarrell
Copy link
Member

Additional note the snapshots getting broken by this test is actually referenced in #1096. I just haven't had time to tweak those tests which use --all to use an isolated directory.

The instrument --in-place test now runs in a copy of the instrument in place test directory, this means that running tests locally doesn't change your local environment.
@coveralls
Copy link

coveralls commented Jul 22, 2019

Coverage Status

Coverage increased (+0.4%) to 97.849% when pulling 6b1dbc9 on AndrewFinlay:instrument-overwrite into c358ce1 on istanbuljs:master.

Copy link
Member

@coreyfarrell coreyfarrell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the delay of review, once the console output is removed I'll be happy to merge this.

lib/commands/instrument.js Outdated Show resolved Hide resolved
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