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

--all will create duplicate report entries on win32 #191

Open
j03m opened this issue Jan 23, 2020 · 4 comments
Open

--all will create duplicate report entries on win32 #191

j03m opened this issue Jan 23, 2020 · 4 comments
Assignees

Comments

@j03m
Copy link
Collaborator

j03m commented Jan 23, 2020

  • Version: 7.0.0
  • Platform: win32

This is a bit of a "canned" issue because it assumes that #183 is fixed upstream. But when testing out my fix istanbuljs/test-exclude#44 I noticed there is an additional path casing issue in c8 related to the implementation of --all.

On windows, using local files it is possible for v8 to generate file names like: d:\path\to\my\file.js
note the lower case drive letter.

But later when find --all files the node api post call to resolve will return the same string paths with a upper case drive letter. This is what originally caused #183.

However, once #183 is fixed and test-exclude's shouldInstrument returns true for case mismatches, this additional mismatch in c8's fileIndex Set which we use to track if a file has been seen causes duplicates in the final report:

For example the output might be as follows:

  file1.js     |       0 |        0 |       0 |       0 | 1-36              
  file1.js     |     100 |      100 |     100 |     100 |                   
  file2.js     |       0 |        0 |       0 |       0 | 1-106             
  file2.js     |   90.57 |   

My current thoughts were to replace fileIndex the Set with an overridden Set that would toLowerCase keys when win32 is detected.

@j03m j03m self-assigned this Jan 23, 2020
@j03m
Copy link
Collaborator Author

j03m commented Jan 23, 2020

That seems to work but I'm having a hard time figuring out how to flex it in a test :/

@bcoe
Copy link
Owner

bcoe commented Jan 24, 2020

@j03m I'd be fine with detecting the environment, and having a test case that only runs in Windows.

@j03m
Copy link
Collaborator Author

j03m commented Jan 24, 2020

@bcoe cool. I'm sort of working on that. Testing it seems to involve changing more code to mock and inject dependencies then I would like. Super messy :/. Trying to figure out a better way.

@j03m
Copy link
Collaborator Author

j03m commented Jan 25, 2020

#192 will fix

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

No branches or pull requests

2 participants