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

Allow eager instantiation of instrumenter #490

Merged
merged 1 commit into from
Jan 17, 2017
Merged

Conversation

evantorrie
Copy link
Contributor

@evantorrie evantorrie commented Jan 13, 2017

This change adds an --eager option to nyc which forces instantiation
of the instrumentation library prior to walking any of the files.

Addresses #491

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 9e77282 on evantorrie:eager into ** on istanbuljs:master**.

@JaKXz
Copy link
Member

JaKXz commented Jan 16, 2017

@bcoe this is the PR for #491 -

Do you guys think it's possible/valid to write a test for this? A full acceptance test with a library that mocks requires would be thorough but possibly overkill.

@bcoe
Copy link
Member

bcoe commented Jan 16, 2017

@evantorrie this is neat, just reading the documentation on ava; we should potentially add a note, with a reference to the caching-transform docs in the README I think. Also, I agree with @JaKXz that figuring out a test would be smart.

@bcoe
Copy link
Member

bcoe commented Jan 16, 2017

@evantorrie another thought comes to mind; is there any reason to not always enable eager? I'm correct in thinking that it's a noop, and should improve performance?

CC: @jamestalmage, @novemberborn

@novemberborn
Copy link
Contributor

is there any reason to not always enable eager? I'm correct in thinking that it's a noop, and should improve performance?

It'll decrease performance in case the required modules have previously been instrumented. That's what CachingTransform is there for, so we don't have to load Istanbul and all it's dependencies on every test run.

@bcoe
Copy link
Member

bcoe commented Jan 16, 2017

@JaKXz realizing now that this is a feature of caching-transform, I think we can probably do without the test; trusting that it's sufficiently tested in the dependent library.

@evantorrie this is looking good to me; this is also enough of a power feature that I could be convinced that the command line argument is sufficient documentation; perhaps we could switch it to:

instantiate the instrumenter at startup (see https://git.io/vMKZ9)

I'll also be quite curious to see the benchmarks of using both approaches on a few large projects after we land this -- given that we have some expensive dependencies like babel, I wonder if average projects will see some performance gains (we might want to enable this by default at some point).

Some mocking libraries hook require and warn when there is a required
module which has not been explicitly registered (e.g. mockery).  It is
standard procedure to enable these mock hooks in a unit test prior to
requiring the code under test.

With lazy instantiation of the instrumenter, all of the instrumenter
library's `require`s generate warnings at test time.

This change adds an `--eager` option to nyc which forces instantiation
of the instrumentation library prior to walking any of the files, thus
eliminating any warnings from mockery.
@evantorrie
Copy link
Contributor Author

evantorrie commented Jan 17, 2017

I've changed the command line description to reference https://git.io/vMKZ9.

Per discussion above, my use-case for this is mostly in running continuous integration via Jenkins which has a fresh checkout from source on each build. Thus, I never get any benefit from the caching transforms (although obviously I recognize you do if this is run in your dev directory/env).

@bcoe

@coveralls
Copy link

coveralls commented Jan 17, 2017

Coverage Status

Changes Unknown when pulling df3428e on evantorrie:eager into ** on istanbuljs:master**.

@bcoe
Copy link
Member

bcoe commented Jan 17, 2017

@evantorrie looks great, I'll look into landing this ASAP.

@bcoe bcoe merged commit 8b58c05 into istanbuljs:master Jan 17, 2017
@bcoe
Copy link
Member

bcoe commented Jan 17, 2017

@evantorrie I've released this feature in the next release of nyc. I would love help testing:

npm cache clear; npm i nyc@next

Look forward to more contributions in the future, thanks! 🎉

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.

5 participants