Implemented combine coverprofile in the case of rerun failed

[yihuaf]

Fix #274

As discussed in #274, when gotestsum rerun failed test cases, the coverprofile gets overridden by the each calls to the underlying go test, causing coverprofile to per in correct.

I tested the code with the repros provided in #274: https://github.com/neiser/gotestsum-rerun-dependent-subtests/tree/master

The code is just a proof of concept. If the general approach is acceptable, then I will go ahead and clean up the PR with necessary unit tests and comments.

The general approach taken in this PR:

• if coverprofile flag is discovered, we parse the flag to get the file path.
• in normal case, we pass the coverprofile directly to go test unmodified.
• in the case that we need to rerun the failed test cases, we modify the coverprofile flag with a new file name.
• after the run, we combine the new coverprofile output with the original file path passed in.

[yihuaf]

Looks like test-windows is broken unrelated to my change. I tried tip of the tree and the test still fails.

=== Failed
=== FAIL: cmd TestE2E_RerunFails/first_run_has_errors,_abort_rerun (0.39s)
    main_e2e_test.go:69: assertion failed: 
        --- expected
        +++ actual
        @@ -1,6 +1,11 @@
        +FAIL testjson/internal/broken
        +
        +=== Failed
        +=== FAIL: testjson/internal/broken 
        +FAIL   gotest.tools/gotestsum/testjson/internal/broken [build failed]
         
         === Errors
         ../testjson/internal/broken/broken.go:5:21: undefined: somepackage
         
        -DONE 0 tests, 1 error
        +DONE 0 tests, 1 failure, 1 error

[dnephin]

Thank you for the PR! I appreciate the contribution.

I had a quick look and overall it sounds reasonable to me. I agree the test failure seems unrelated.

I will find some more time to review in more detail. Some test coverage would also be appreciated!

[dnephin]

I think we may need to copy the license file
(https://github.com/wadey/gocovmerge/blob/master/LICENSE) into internal/coverprofile to comply with the license.

[yihuaf]

I added the LICENSE to the file header since it is only the file we copied over. Please let me know if this is good enough.

[dnephin]

It might be better to wait until all the attempts are complete and then combine all the files. That way we aren't re-reading the main file each time.

Would that work?

[yihuaf]

Yes, this would work and better.

One caveat is in the case of failure. When go test fails, the coverage profile still contain information up to the test failures. In the case of failure re-run, if we error out, we should still have 1 valid coverage profile and not leave profiles un-combined. So leaving all the combine to the end may not be desirable.

If reading the main profile multiple times is a concern, I can parse the main profile 1 time upfront and save the combined profile in memory each time when combine the re-run profile. Would this work? Not too sure if the performance gain from this optimization would justify the complexity increase.

[yihuaf]

After some more thoughts, I implemented the logic as follow:

• each rerun will write the coverage profile into it's own file
• after each rerun, read the rerun coverage profile into memory and delete the file
• after all rerun is done before success, we combine all the rerun coverage profile with the main profile

This achieve 2 goals:

• Only reading the main coverage profile once.
• The coverage is valid upto the latest failed rerun and the rerun cover profile file is cleaned up

PTAL and let me know what you think.

[gaby]

@dnephin I was trying to do a PR to update your dependencies but tests are failing locally with the same error posted by @yihuaf

[yihuaf]

@dnephin Please let me know if there is anything you would like me to follow up :)

[yihuaf]

@dnephin Let me know if there is anything else you would like me to fix :)