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

Support in-place instrumentation (again) #1135

Closed
DanTup opened this issue Jun 26, 2019 · 13 comments
Closed

Support in-place instrumentation (again) #1135

DanTup opened this issue Jun 26, 2019 · 13 comments

Comments

@DanTup
Copy link

DanTup commented Jun 26, 2019

It used to be possible to instrument in-place, but this is rejected since #1007.

It's not clear from the PR why, but it broke things for me, and trying to use a different folder is complicated (because there are many paths into the JS folder).

I understand this can be risky, but in my case it's just the transpiled output of typescript. It'd be nice if this was supported, even if you had to pass an extra flag to opt-in to it.

@coreyfarrell
Copy link
Member

We require a link to a repository showing an example project where this not working.

@DanTup
Copy link
Author

DanTup commented Jul 3, 2019

All you need to do is run npm instrument with the same input/output on any project. The code checks for this and exits with an error (as of the PR linked above):

if (argv.output && (path.resolve(argv.cwd, argv.input) === path.resolve(argv.cwd, argv.output))) {
console.error(`nyc instrument failed: cannot instrument files in place, <input> must differ from <output>`)
process.exitCode = 1
return

I haven't committed the upgrade that breaks this because I need my coverage to work in the meantime. This is a feature request to support this again.

@coreyfarrell
Copy link
Member

@AndrewFinlay do you have any thoughts? I know this needs to be blocked if --delete is enabled, outside of that I'm just not sure. It seems dangerous to rewrite files in-place, though it appears that @babel/cli allows it (a bug or a feature?).

@DanTup If you provided a link to a repository which uses in-place instrumentation I might be able to suggest a better way. Maybe nyc instrument --complete-copy=true src dest could work for you.

@DanTup
Copy link
Author

DanTup commented Jul 3, 2019

It seems dangerous to rewrite files in-place

Many things are dangerous, but making them impossible is really restrictive. This feature used to work, and I'm running it on the output of TypeScript so there's really no problem. You could make a --force or --some-really-scary-flags-that-acknowledge-the-danger flag if you're concerned people won't understand - but taking it away just means people will hang around on old versions if there's no easy way to switch to out-of-place instrumentation.

I did try for some time to make the new version work, but there many places reference this folder (such as all the VS Code test entry points) so I gave up and just rolled back (I'd already spent several hours trying to get this working again after adding webpack - after spending many hours getting it working the first time!).

My setup is super-janky (for a whole host of reasons, involving VS Code, webpack, mismatched source map paths between packed/unpacked and some weird behaviour from nyc depending on which folder it's executed from within):

https://github.com/Dart-Code/Dart-Code/blob/7c64f5e3de13afa1c64f4b0a56877e74d7264c3a/package.json#L1454-L1468

https://github.com/Dart-Code/Dart-Code/blob/7c64f5e3de13afa1c64f4b0a56877e74d7264c3a/src/test/remap_coverage.ts

https://github.com/Dart-Code/Dart-Code/blob/7c64f5e3de13afa1c64f4b0a56877e74d7264c3a/src/test/test_runner.ts#L10-L22

Maybe it can be simplified, but everything I tried (even just trying to eliminate things like all those wonky cd foo/foo && xxx && cd ../.. calls would break things 😞

@AndrewFinlay
Copy link
Contributor

Hi @DanTup & @coreyfarrell,

I've been a bit busy of late so I've had to put my nyc work to the side. However it turns out the last thing I was working on back in mid May was a switch to allow in-place instrumentation, so I guess that means I agree with Dan that it's needed. It'll need a bit more work to get it finished with tests, I'll try and find some time to finish it off today. It should be suitable for a minor release when done.

@coreyfarrell, I was originally thinking of naming the switch 'overwrite' with an alias of '-o', but I think I like Dan's name of 'force' better, your thoughts?

@coreyfarrell
Copy link
Member

@AndrewFinlay semver-major changes have already been committed to master, we're currently working towards nyc@15.0.0. I'm thinking nyc instrument --allow-in-place foo foo. Throw an error for same directory if --allow-in-place is not enabled or if --delete is enabled. I'd rather we don't have a short alias for this option. overwrite isn't great because nyc instrument src dest will already overwrite dest/file.js if it exists and src/file.js is processed.

@DanTup
Copy link
Author

DanTup commented Jul 4, 2019

This all sounds good to me! @AndrewFinlay it's not urgent for me to upgrade, so don't worry about rushing to finish this for me, as long as it's coming in the future it prevents me getting stuck on an old version :-) Thanks!

@AndrewFinlay
Copy link
Contributor

@coreyfarrell, I was writing some documentation for this when it struck me that specifying the output directory for this command is a little redundant. So I'm wondering if the command should still be
nyc instrument --allow-in-place <source> <dest> or if nyc instrument --in-place <source> was better?

@papb
Copy link

papb commented Jul 7, 2019

I would also like to see this feature, and I agree with @AndrewFinlay that nyc instrument --in-place <source> is the best option!

@coreyfarrell
Copy link
Member

@AndrewFinlay --in-place and throwing an error if a second argument is given probably makes sense. Also I forgot to mention in addition to --delete I think --complete-copy should probably exit with an error if combined with --in-place.

@stale
Copy link

stale bot commented Sep 5, 2019

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Sep 5, 2019
@papb
Copy link

papb commented Sep 6, 2019

-- comment to counter stalebot --

@stale stale bot removed the stale label Sep 6, 2019
@coreyfarrell
Copy link
Member

The fix for this was merged in #1149 (it was an accident that we didn't close this ticket when that merge occurred). For progress on a new release please subscribe to #1104, this is where I will announce any beta releases and eventually the final release of nyc 15.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants