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: Update caching-transform options. #873

Merged
merged 1 commit into from Jun 29, 2018

Conversation

coreyfarrell
Copy link
Member

When updating caching--transform it was missed that the hash option
has been refactored. This means that two files with identical contents
would be treated as identical.

Add a test to verify that two sources with identical contents are
both instrumented.

When updating caching--transform it was missed that the `hash` option
has been refactored.  This means that two files with identical contents
would be treated as identical.

Add a test to verify that two sources with identical contents are
both instrumented.

assert.equal(file1(), file2())

const cov = (new Function('return this.__coverage__'))()
Copy link
Member Author

Choose a reason for hiding this comment

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

I'm open to better ways to test that the coverage data includes the 3 files.

Copy link
Member

Choose a reason for hiding this comment

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

@coreyfarrell this assertion seems reasonable to me but, out of curiosity, what would the behavior have been before your fixes to the caching transform, would we have only had one of the two files listed in the coverage object?

mind testing running this test prior to your fix and confirming that it breaks as expected?

Copy link
Member Author

Choose a reason for hiding this comment

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

I did test it, without the fixes to caching-transform this test would not record results for identical-file2.js. This is because both identical files have the same hash value so caching-transform gave the instrumented code from file1 to file2. This meant file2 recorded it's results to file1.

@coreyfarrell coreyfarrell requested a review from bcoe June 27, 2018 23:01
Copy link
Member

@bcoe bcoe left a comment

Choose a reason for hiding this comment

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

this is looking good to me 👍

@bcoe bcoe merged commit 52b69ef into istanbuljs:master Jun 29, 2018
@coreyfarrell coreyfarrell deleted the fix-caching-transform branch July 12, 2018 18:14
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.

None yet

2 participants