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

Fix coverage when require cache is reset #510

Merged
merged 1 commit into from Jan 12, 2016

Conversation

@tjmehta
Copy link
Contributor

tjmehta commented Jan 11, 2016

Require-mocking libraries (such as proxyquire) and other modules may reset the require cache and enable requiring the same file multiple times.

@tjmehta

This comment has been minimized.

Copy link
Contributor Author

tjmehta commented Jan 11, 2016

Fixes #505

done()
})

it('does not reset file coverage', function (done) {

This comment has been minimized.

Copy link
@geek

geek Jan 11, 2016

Member

If you remove the lab.before and lab.after and put the code that changes the cache directly into the test, does it work?

This comment has been minimized.

Copy link
@tjmehta

tjmehta Jan 11, 2016

Author Contributor

yes it does, but if the test fails due to some runtime error the require cache will not restore.

@geek geek self-assigned this Jan 11, 2016

const cacheBackup = require.cache; // backup require cache

lab.before((done) => {

This comment has been minimized.

Copy link
@geek

geek Jan 11, 2016

Member

please move the setup and takedown into the test "does not reset file coverage"

This comment has been minimized.

Copy link
@tjmehta

tjmehta Jan 11, 2016

Author Contributor

Should I leave the describe?

@tjmehta tjmehta force-pushed the tjmehta:proxyquire-coverage-fix branch from 1dea288 to 96bd9fc Jan 11, 2016
@tjmehta

This comment has been minimized.

Copy link
Contributor Author

tjmehta commented Jan 11, 2016

Removed before and after, lmk if anything else needs to be changed.


describe('Clear require cache', () => {

it('does not reset file coverage', function (done) {

This comment has been minimized.

Copy link
@geek

geek Jan 11, 2016

Member

I ran this without the fix and it passes. Can you create a test that fails without this fix

This comment has been minimized.

Copy link
@tjmehta

tjmehta Jan 12, 2016

Author Contributor

I see a mistake. Sorry about that.

This comment has been minimized.

Copy link
@tjmehta

tjmehta Jan 12, 2016

Author Contributor

Fixed.

This comment has been minimized.

Copy link
@gergoerdosi

gergoerdosi Jan 12, 2016

Contributor

Make it an arrow function.

@tjmehta tjmehta force-pushed the tjmehta:proxyquire-coverage-fix branch from 96bd9fc to 466cd0a Jan 12, 2016
@tjmehta tjmehta force-pushed the tjmehta:proxyquire-coverage-fix branch from 466cd0a to 62a0cdb Jan 12, 2016
@tjmehta

This comment has been minimized.

Copy link
Contributor Author

tjmehta commented Jan 12, 2016

Changed function to arrow

@geek geek added the bug label Jan 12, 2016
@geek geek added this to the 8.0.2 milestone Jan 12, 2016
@geek

This comment has been minimized.

Copy link
Member

geek commented Jan 12, 2016

@tjmehta thanks for the fix, looks good. Going to publish as part of 8.0.2

geek added a commit that referenced this pull request Jan 12, 2016
Fix coverage when require cache is reset
@geek geek merged commit cd640d5 into hapijs:master Jan 12, 2016
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@tjmehta tjmehta deleted the tjmehta:proxyquire-coverage-fix branch Mar 15, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.