fix: do not override NODE_V8_COVERAGE if set (#70) #78

kilianc · 2019-04-20T19:33:38Z

WIP, please do not merge yet

kilianc · 2019-04-20T19:41:09Z

How do you want me to test this properly? I think is messing up the snapshots too

shinnn

please do not merge yet

OK.

shinnn · 2019-04-20T19:47:17Z

test/integration.js

  })

+  it('supports exeternally set NODE_V8_COVERAGE', () => {
+    process.env.NODE_V8_COVERAGE = './coverage/tmp_'


Instead of overriding NODE_V8_COVERAGE of the whole process, use spawnSync() option to modify only this child process' environment.

shinnn · 2019-04-20T19:48:55Z

test/parse-args.js

+      process.argv = ['node', 'c8', '--foo=99', 'my-app', '--help']
+      const argv = buildYargs().parse(hideInstrumenteeArgs())
+      argv.tempDirectory.endsWith('/coverage/tmp_').should.be.equal(true)
+      process.env.NODE_V8_COVERAGE = ''


I assume the original value is not '', as this test is run with c8.

I can delete but that would create a side effect like setting it to ''. I can use beforeEach and store the original value. I am only doing it that particular test now.

shinnn · 2019-04-20T20:49:27Z

While reviewing this, I found the existing test results are too fragile to add new specs... the snapshot of the first test affects the second one, the second affects the third, and so on.

I think we should make each tests more independent before implementing new features.

@kilianc, thanks for starting this work.

kilianc · 2019-04-20T21:00:41Z

Ok, I'll keep this parked here. @ me when I can continue!

shinnn · 2019-04-20T21:21:48Z

we should make each tests more independent before implementing new features.

#79

shinnn · 2019-04-28T19:59:59Z

@kilianc Can you rebase this branch from master?

bcoe · 2019-04-30T18:13:39Z

👋 @kilianc it seems like this has stalled a bit, still a feature you need?

Could you explain your use-case for me, I don't quite understand why you'd want to run c8 without setting the variable that allows it to start collecting coverage.

bcoe · 2019-05-02T03:14:14Z

@kilianc this is looking pretty good to me, what work would you still like to do?

shinnn · 2019-05-02T03:44:41Z

bin/c8.js

    }

-    process.env.NODE_V8_COVERAGE = argv.tempDirectory
+    if (!process.env.NODE_V8_COVERAGE) {


Which should c8 take precedence, when both NODE_V8_COVERAGE environment variable and --tmp-directory CLI flag are provided? The current implementation will choose NODE_V8_COVERAGE in this case.

--tmp-directory should win, but we default it to NODE_V8_COVERAGE if not set, we can get rid of this if statement probably

we can get rid of this if statement probably

👍

kilianc · 2019-05-06T20:56:06Z

Could you explain your use-case for me, I don't quite understand why you'd want to run c8 without setting the variable that allows it to start collecting coverage.

Quite the opposite! I am trying to give full ownership of the env to the caller, in my case CI. Instead of having to re-pass in the env as arg just to have it set again.

I hope that makes sense

kilianc · 2019-05-06T21:04:22Z

BTW, not sure what is going on with master but probably someone force-pushed because I can't pull anymore. I am trying to fix my branches and force push on this PR.

kilianc · 2019-05-06T21:47:41Z

I resolved my git issues but the test suite is still not passing. Will work on it this weekend, but I think the test suite still has some isolation problem. Cheers!

bcoe · 2019-05-07T00:20:06Z

@kilianc once I read up on your reasoning for this feature, I totally understand what you're going for 👍 excited to see this over the finish line; let me know if there's anything I can do to help.

shinnn · 2019-05-07T13:42:48Z

test/integration.js

+    const { output } = spawnSync(nodePath, [
+      c8Path,
+      '--exclude="test/*.js"',
+      '--clean=false',


I think the test suite still has some isolation problem.

Use --temp-directory option here to write coverage reports to a separate directory.

@shinnn the goal of this test is indeed to use the env and not --temp-directory to set the path. Look at line 38!

Ah, indeed.

kilianc · 2019-05-08T02:05:50Z

@bcoe thanks, I think this is ready. Happy to fix anything that doesn't meet expectations :)

bcoe · 2019-05-13T23:16:23Z

@shinnn how are you feeling about this PR?

bcoe · 2019-05-16T17:32:21Z

@kilianc this look reasonable to me, but all the tests appear to be failing, any thoughts?

kilianc · 2019-05-20T21:13:24Z

@bcoe locally it was all passing, I'll try to rebase again! 🍺

bcoe · 2019-06-09T17:27:44Z

@kilianc just wanted to bump this up, haven't unfortunately had the cycles to dig into the issues myself.

Still something you'd like to land?

kilianc · 2019-06-09T20:33:02Z

@bcoe it is, it's just bandwidth. I'll do my best to get this done in the next week or so! Thanks for the bump :)

bcoe · 2019-06-23T20:29:27Z

closing in favor of #122; I think there was just a bad entry in the snapshot, from prior runs of your tests.

kilianc force-pushed the fix/default-env branch 2 times, most recently from 9eb284b to 71c44e1 Compare April 20, 2019 19:38

kilianc changed the title ~~fix: do not override NODE_V8_COVERAGE if set~~ fix: do not override NODE_V8_COVERAGE if set (#70) Apr 20, 2019

kilianc force-pushed the fix/default-env branch 2 times, most recently from 0706789 to f4c9d66 Compare April 20, 2019 19:47

shinnn suggested changes Apr 20, 2019

View reviewed changes

kilianc force-pushed the fix/default-env branch from f4c9d66 to 976b33c Compare April 20, 2019 20:36

shinnn reviewed May 2, 2019

View reviewed changes

fix: do not override NODE_V8_COVERAGE if set (bcoe#70)

da41f80

kilianc force-pushed the fix/default-env branch from cc6b673 to da41f80 Compare May 6, 2019 21:56

shinnn suggested changes May 7, 2019

View reviewed changes

bcoe added the blocked For blocked PRs waiting for something else to continue with review or land label May 19, 2019

bcoe mentioned this pull request Jun 23, 2019

chore: seeing kilianc's work over finish line #122

Merged

bcoe closed this Jun 23, 2019

bcoe mentioned this pull request Jun 24, 2019

chore: release 5.0.2 #123

Merged

fix: do not override NODE_V8_COVERAGE if set (#70) #78

fix: do not override NODE_V8_COVERAGE if set (#70) #78

Uh oh!

Conversation

kilianc commented Apr 20, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kilianc commented Apr 20, 2019

Uh oh!

shinnn left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shinnn commented Apr 20, 2019

Uh oh!

kilianc commented Apr 20, 2019

Uh oh!

shinnn commented Apr 20, 2019

Uh oh!

shinnn commented Apr 28, 2019

Uh oh!

bcoe commented Apr 30, 2019

Uh oh!

bcoe commented May 2, 2019

Uh oh!

shinnn May 2, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kilianc commented May 6, 2019

Uh oh!

kilianc commented May 6, 2019

Uh oh!

kilianc commented May 6, 2019

Uh oh!

bcoe commented May 7, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kilianc commented May 8, 2019

Uh oh!

bcoe commented May 13, 2019

Uh oh!

bcoe commented May 16, 2019

Uh oh!

kilianc commented May 20, 2019

Uh oh!

bcoe commented Jun 9, 2019

Uh oh!

kilianc commented Jun 9, 2019

Uh oh!

bcoe commented Jun 23, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

kilianc commented Apr 20, 2019 •

edited

Loading

shinnn left a comment •

edited

Loading

shinnn May 2, 2019 •

edited

Loading