search

mweibel / lcov-result-merger

Merges multiple lcov results into one
MIT License
102 stars 22 forks source link

Next release #51

Closed hugobessa closed 1 year ago

hugobessa commented 2 years ago

Do we have plans for rolling the next release? Is there any blocker? Any help wanted?

We have breaking changes like dependency updates and dropping support to older versions on Node. Should we bump it to 4.0.0?

mdeanjones commented 2 years ago

Thanks for the reminder @hugobessa. We'll need to summon the keeper of the keys, @mweibel, to perform the necessary ceremonies.

This definitely warrants a major bump to 4.0.

mweibel commented 2 years ago

@mdeanjones running tests locally and one failed:

 2 passing (183ms)
  1 failing

  1) lcovResultMerger CLI
       should optionally prepend source file lines with corrected pathing:

      AssertionError: expected 'SF:./test/fixtures/coverage-subfolder…' to equal 'SF:./test/fixtures/coverage-subfolder…'
      + expected - actual

      -SF:./test/fixtures/coverage-subfolder/b/src/index.js
      +SF:./test/fixtures/coverage-subfolder/a/src/index.js
       DA:1,1
       end_of_record
      -SF:./test/fixtures/coverage-subfolder/b/src/Foo.js
      +SF:./test/fixtures/coverage-subfolder/a/src/Foo.js
       DA:1,1
       DA:4,1
       DA:5,17
       DA:8,8
--
       DA:28,8
       DA:33,1
       DA:34,9
       DA:37,1
      -BRDA:24,1,0,2
      +BRDA:24,1,0,-
       BRDA:24,1,1,8
       BRDA:24,1,2,-
       end_of_record
      -SF:./test/fixtures/coverage-subfolder/a/src/index.js
      +SF:./test/fixtures/coverage-subfolder/b/src/index.js
       DA:1,1
       end_of_record
      -SF:./test/fixtures/coverage-subfolder/a/src/Foo.js
      +SF:./test/fixtures/coverage-subfolder/b/src/Foo.js
       DA:1,1
       DA:4,1
       DA:5,17
       DA:8,8
--
       DA:28,8
       DA:33,1
       DA:34,9
       DA:37,1
      -BRDA:24,1,0,-
      +BRDA:24,1,0,2
       BRDA:24,1,1,8
       BRDA:24,1,2,-
       end_of_record

      at Context.<anonymous> (test/lcov-result-merger-cmd-spec.js:71:19)
      at processTicksAndRejections (node:internal/process/task_queues:96:5)

could you please double-check?

mdeanjones commented 2 years ago

Looking now @mweibel; I can reproduce locally as well. First impression is that there's a change in the newline character(s), but I'm not quite sure. Something subtle is definitely different between Ubuntu and OSX.

hugobessa commented 2 years ago

I'll see what I can do. Unfortunately, I don't have access to OSX to reproduce the issue and debug.

WikiRik commented 2 years ago

I think this test is flaky on Ubuntu (maybe others as well). Or could you consistently reproduce this issue?

dzzk commented 2 years ago

Do not know how it was before but if you need MacOS guy - here I am.

"lcov-result-merger": "^3.3.0"

I did not provide correct glob for search on start, since I did supposed I'm in a right place.

My report is below:

 yarn lcov-result-merger lcov.info outFile.lcov
yarn run v1.22.19
$ ../Users/../node_modules/.bin/lcov-result-merger lcov.info outFile.lcov
node:events:371
      throw er; // Unhandled 'error' event
      ^

Error: File not found with singular glob: /Users/../lcov.info (if this was purposeful, use `allowEmpty` option)
    at Glob.<anonymous> (/Users/../node_modules/glob-stream/readable.js:84:17)
    at Object.onceWrapper (node:events:514:26)
    at Glob.emit (node:events:394:28)
    at Glob._finish (/Users/../node_modules/glob/glob.js:194:8)
    at done (/Users/../node_modules/glob/glob.js:179:14)
    at Glob._processSimple2 (/Users/../node_modules/glob/glob.js:685:12)
    at /Users/../node_modules/glob/glob.js:673:10
    at Glob._stat2 (/Users/../node_modules/glob/glob.js:769:12)
    at lstatcb_ (/Users/../node_modules/glob/glob.js:761:12)
    at RES (/Users/../node_modules/inflight/inflight.js:31:16)
Emitted 'error' event on DestroyableTransform instance at:
    at Pumpify.emit (node:events:394:28)
    at Pumpify.Duplexify._destroy (/Users/../node_modules/duplexify/index.js:191:15)
    at /Users/../node_modules/duplexify/index.js:182:10
    at processTicksAndRejections (node:internal/process/task_queues:78:11)
error Command failed with exit code 1.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.

Than I changed glob patter to right one:

 yarn lcov-result-merger coverage/**/lcov.info outFile.lcov
yarn run v1.22.19
$ /Users/../node_modules/.bin/lcov-result-merger coverage/integration/lcov.info outFile.lcov
✨  Done in 0.68s.
mweibel commented 2 years ago

thanks for the ping @dzzk. Not sure if you wanted to open a new issue though?

I think this test is flaky on Ubuntu (maybe others as well). Or could you consistently reproduce this issue?

I tested again and it is indeed flaky. However, looking more closely at the test failure, I believe the reason is because the order of files in the result is not always the same. @hugobessa could this be because of the latest changes?

@mdeanjones not sure how you want to proceed - do you want to try fixing the test or do you want to ignore it for now and release version 4.0.0?

hugobessa commented 2 years ago

I've made many changes to the file processing part as I completely removed vinyl-fs as a dependency. So, yes, the latest change can be causing the flakiness. I'll try to reproduce the issue here and fix it.

mdeanjones commented 1 year ago

Sorry for the long silence @mweibel and @hugobessa; I've been away from my keyboard for awhile. Correct me if I am wrong, but I don't believe that the file order being non-deterministic would effect how the resulting LCOV is interpreted. That said, it'd be good to know if it is indeed the culprit.

I haven't worked with fast-glob much, but I am curious to see if its concurrency capability might have something to do with this. Will take a look today.

mweibel commented 1 year ago

no it shouldn't have an influence - fixing the test to ignore the order of files would be a viable option too IMO

mdeanjones commented 1 year ago

Not sure why I was stroking my chin so much - "if it is indeed the culprit" - when the test output is really clear on that point. Maybe I came back from vacation too soon.

It looks like there are some small behavioral differences between vinyl and fast-glob that may be getting in the way here. vinyl File objects default to an absolute path, while fast-glob's do not. Configuring fast-glob { absolute: true } appears to clear the issue up on OSX.

https://github.com/mdeanjones/lcov-result-merger/blob/0d8f6a2e13fc6c3dcd16e830b9429248ed372b7d/bin/lcov-result-merger.js#L38

mdeanjones commented 1 year ago

Not sure why I was stroking my chin so much - "if it is indeed the culprit" - when the test output is really clear on that point. Maybe I came back from vacation too soon.

It looks like there are some small behavioral differences between vinyl and fast-glob that may be getting in the way here. vinyl File objects default to an absolute path, while fast-glob's do not. Configuring fast-glob { absolute: true } appears to clear the issue up on OSX.

https://github.com/mdeanjones/lcov-result-merger/blob/0d8f6a2e13fc6c3dcd16e830b9429248ed372b7d/bin/lcov-result-merger.js#L38

Disregard this. Run enough times and you'll still see A <-> B flip back and forth. In fact, fast glob points out in the opening paragraph of its README that "meanwhile results are returned in arbitrary order". Looks like there are a couple of possible paths forward:

  1. @mweibel's thought on modifying the tests to not care about ordering is looking solid, but might be a significant lift.
  2. replace fast-glob with something else that can guarantee ordering.
  3. keep fast-glob, and manage the ordering ourselves.

Thoughts, @hugobessa?

mweibel commented 1 year ago

Release 4.0.0 is out. Thanks everyone.