Fix race condition in coverage tests #423
Conversation
|
Thanks for looking at this, will try to review today. |
integration/coverage_test.go
Outdated
| @@ -59,23 +58,24 @@ var _ = Describe("Coverage Specs", func() { | |||
| Eventually(session).Should(gexec.Exit(0)) | |||
|
|
|||
| // Check that the correct file was created | |||
| _, err := os.Stat("./_fixtures/coverage_fixture/coverage.txt") | |||
| coverFile := "./_fixtures/coverage_fixture/coverage.txt" | |||
| _, err := os.Stat(coverFile) | |||
There was a problem hiding this comment.
Can we use: Expect(coverFile).To(BeARegularFile()) ?
integration/coverage_test.go
Outdated
| Ω(err).ShouldNot(HaveOccurred()) | ||
|
|
||
| os.RemoveAll("./_fixtures/coverage_fixture/coverage_fixture.coverprofile") | ||
| os.RemoveAll(coverFile) |
There was a problem hiding this comment.
WDYT about wrapping these in Expect(os.RemoveAll(coverFile)).To(Succeed()).
I'm torn on this style, on the one hand it's an assertion that isn't really related to the behaviour under test, on the other hand, I've been bitten so many times by debris from previous tests.
There was a problem hiding this comment.
If coverFile is a regular file, can we use os.Remove(coverFile). Seeing RemoveAll in code leads me to believe we are deleting a directory.
integration/coverage_test.go
Outdated
|
|
||
| Eventually(session).Should(gexec.Exit(0)) | ||
|
|
||
| packages := []string{"first_package", "second_package"} | ||
|
|
||
| for _, p := range packages { | ||
| coverFile := fmt.Sprintf("./_fixtures/combined_coverage_fixture/%s/coverage.txt", p) | ||
| coverFile := fmt.Sprintf("./_fixtures/combined_coverage_fixture/%s/coverage1.txt", p) |
There was a problem hiding this comment.
I wonder whether we can come up with more self-explaining names for these files than 1,2,3 etc. e.g. coverage-recursive.txt
I'm also not super clear on why these all have different names rather than different output dirs but I'm definitely missing something.
|
Bump @alamages any thoughts on the review? |
| @@ -89,3 +89,8 @@ func startGinkgo(dir string, args ...string) *gexec.Session { | |||
| Ω(err).ShouldNot(HaveOccurred()) | |||
| return session | |||
| } | |||
|
|
|||
| func removeSuccessfully(path string) { | |||
There was a problem hiding this comment.
Can we just use Expect(os.RemoveAll(path)).To(Succeed()) ? Do we need this extra indirection to another method as well, or is it cause you don't want the assertion in the test block itself?
There was a problem hiding this comment.
Yes, the initial idea was not to have the assertion within the block itself. The PR is still WIP since we started refactoring to make sure all generated coverage files are always removed.
|
@williammartin you can take a look at this PR again now We specify different output coverage files instead of different output directories because:
Let me know if you want any further changes on this, thanks! |
|
Once this PR: #446 is merged I will have to merge with master and adapt the changes. |
|
@alamages gogogo |
|
@williammartin done :D |
alamages
dismissed
williammartin’s stale review
Decided to use removeSuccessfully
|
Can you fix up the commits as well please? I'd rather not have a git tree containing a bunch of |
alamages
force-pushed
the
coverage-tests-fix
branch
3 times, most recently
from
e1f0152
to
3f8c435
Compare
Tests now generate coverprofile files into different paths so now they don't interfere with each other and can be run successfully in parallel. Some extra changes: - Use BeARegularFile() to check for file existance in coverage_test - Wrap every It with a Context to properly remove any generated file - Use removeSuccessfully helper function to cleanup generated files
alamages
force-pushed
the
coverage-tests-fix
branch
from
3f8c435
to
a5a8ff7
Compare
|
@williammartin it should be ok now |
|
LGTM thanks |
Tests now generate coverprofile files into different paths so now they don't
interfere with other and can be run successfully in parallel. Also since
the files now have different names, some extra cleanup was required.
Related issue: #394