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: stop duplicating names on case-insensitive file systems #192

Closed
wants to merge 2 commits into from

Conversation

j03m
Copy link
Collaborator

@j03m j03m commented Jan 25, 2020

I found that on win32, if the v8 script name was set using "legal" (I'm using that term loosely, because it probably shouldn't be done by the embedder), but mismatched casing for a file path, it would result in --all reporting duplicate records for an entry.

For example where node's path.resolve might provide you with c:\src\main.js it would be perfectly fine on windows to tell v8 that the script resource was c:\src\MAIN.js. If you then try to measure coverage with all, the mismatch in casing between v8's blobs and node's file system results cause the system to report duplicate empty records for files that are actually in the blobs, just differ on case.

Prior to my fix test output vs the correct snapshot would like like:

     AssertionError: expected value to match snapshot c8 --all should, given path casing mismatches between path.resolve and v8 output, NOT contain duplicate report entries 1
      + expected - actual

       ",--------------|---------|----------|---------|---------|-------------------
       File          | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s
       --------------|---------|----------|---------|---------|-------------------
      -All files     |   35.29 |    54.55 |      25 |   35.29 |                   
      - vanilla      |   39.13 |       60 |   33.33 |   39.13 |                   
      +All files     |   64.29 |    66.67 |      50 |   64.29 |                   
      + vanilla      |   78.26 |       75 |     100 |   78.26 |                   
         LOADED.js   |   73.68 |    71.43 |     100 |   73.68 | 4,5,16-18
         MAIN.js     |     100 |      100 |     100 |     100 |
      -  loaded.js   |       0 |        0 |       0 |       0 | 1-19              
      -  main.js     |       0 |        0 |       0 |       0 | 1-4               
        vanilla/dir  |       0 |        0 |       0 |       0 |
         unloaded.js |       0 |        0 |       0 |       0 | 1-5
       --------------|---------|----------|---------|---------|-------------------
       ,"

      at Context.<anonymous> (test\integration.js:457:40)
      at processImmediate (internal/timers.js:439:21)
Checklist
  • [ x ] npm test, tests passing
  • [ x ] npm run test:snap (to update the snapshot)
  • [ x ] tests and/or benchmarks are included
  • documentation is changed or added

@coveralls
Copy link

coveralls commented Jan 25, 2020

Pull Request Test Coverage Report for Build 536

  • 28 of 29 (96.55%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.3%) to 94.507%

Changes Missing Coverage Covered Lines Changed/Added Lines %
lib/report.js 2 3 66.67%
Totals Coverage Status
Change from base Build 529: 0.3%
Covered Lines: 586
Relevant Lines: 609

💛 - Coveralls

@j03m j03m changed the title [fix] win32 mismatch of drive casings causes duplicate entries [no merge] [fix] win32 mismatch of drive casings causes duplicate entries Jan 25, 2020
@j03m j03m changed the title [no merge] [fix] win32 mismatch of drive casings causes duplicate entries [fix] win32 mismatch of drive casings causes duplicate entries Jan 25, 2020
@j03m j03m changed the title [fix] win32 mismatch of drive casings causes duplicate entries [fix] win32 mismatch of path casings causes duplicate entries Jan 25, 2020

add (value) {
if (this._win32) {
value = value.toLowerCase()
Copy link
Owner

Choose a reason for hiding this comment

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

My concern is that I think Windows can have case sensitivity enabled? see this article.

I wonder if we could find a way to feature detect case sensitivity, rather than basing it on Windows?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oh I didn't know that. Will review. Thanks!

Copy link

Choose a reason for hiding this comment

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

I looked into that for jest and didn't find a nice way (suggested solutions was writing a file and reading it with different casing, but it had some caveats).

If you do find a nice way, please publish it as a separate module and we can use it in Jest as well 😀

Copy link
Owner

Choose a reason for hiding this comment

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

@SimenB did you publish a module for the approach used by Jest? otherwise I'd be happy to pull together a module that at least exposes the hack.

Copy link

Choose a reason for hiding this comment

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

No, we gave up and just check for platform === 'win32' 😞 So please put together a module! Would be cool if Node itself could expose something...

@bcoe bcoe changed the title [fix] win32 mismatch of path casings causes duplicate entries fix: stop duplicating names on case-insensitive file systems Feb 1, 2020
@j03m
Copy link
Collaborator Author

j03m commented Feb 7, 2020

Just noting an update here - we're going to put this on hold in favor of creating a separate module in istanbul that can detect case sensitivity of the file system. See: istanbuljs/test-exclude#44

@bcoe bcoe added the blocked For blocked PRs waiting for something else to continue with review or land label Feb 7, 2020
@j03m
Copy link
Collaborator Author

j03m commented May 7, 2020

Closing this. Will try to create a module this weekend.

@j03m j03m closed this May 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
blocked For blocked PRs waiting for something else to continue with review or land
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants