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

feat!: Drop node.js 6, upgrade dependencies #1134

Merged
merged 2 commits into from Jul 12, 2019

Conversation

coreyfarrell
Copy link
Member

No description provided.

@coveralls
Copy link

coveralls commented Jun 20, 2019

Coverage Status

Coverage decreased (-0.01%) to 96.842% when pulling 6bc104f on coreyfarrell:node8 into 421c5bd on istanbuljs:master.

@lukeapage
Copy link

lukeapage commented Jun 22, 2019

(edit) I don't think its a bug in this PR. in fact I have no idea why I hit the problem, seems for me I need to specify the path directly in spawn sync rather than just rely on it knowing nyc. Maybe I don't have nyc installed globally? Sorry for the confusion.


I don't know if its related to this change or istanbul, but I made instructions to test the master of the instabuljs mono-repo with it dog-fooding its own source, for trying out report changes:

https://gist.github.com/lukeapage/c7844b3bd7fab7eed260bfef387848b9

After fixing the none report bug though, it still doesn't work.

It looks like the merge command on this pr as run here:
https://github.com/istanbuljs/istanbuljs/blob/master/monorepo-merge-reports.js#L19

doesn't merge, but generates the reports as it might do with normal report generation.

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, as soon as those Node 8 tests are passing 👍

spread, how fancy.

Copy link
Member

@JaKXz JaKXz left a comment

Choose a reason for hiding this comment

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

👍 from me; like Ben said, when the tests are passing it's LGTM!

@coreyfarrell
Copy link
Member Author

The issue is not node.js 8, it's Windows (yes, how shocking). I suspect the problem is due to istanbuljs/istanbuljs#422, not sure if this is exposing a failure in test-exclude or if the nyc tests are just wrong for Windows. It's unlikely I'll be able to work on this before next week.

@coreyfarrell
Copy link
Member Author

Finally got around to dealing with the test. Basic issue is that testExclude.shouldInstrument no longer accepts / based paths on Windows, the root path needs to be drive based (thus C:// or similar). This is a non-issue in practice, the problem is that when the test ran nyc.exclude.shouldInstrument('/cwd/foo.js', 'foo.js'), this is something that would never happen in real life.

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

5 participants