Skip to content

Fix race condition in coverage tests #423

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.

Sign up for GitHub

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

Merged
williammartin merged 1 commit into from
Apr 11, 2018
Merged

Fix race condition in coverage tests #423

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
192 changes: 96 additions & 96 deletions integration/coverage_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,115 +1,124 @@
package integration_test

import (
"os"
"os/exec"

"fmt"

. "github.com/onsi/ginkgo"
. "github.com/onsi/gomega"
"github.com/onsi/gomega/gbytes"
"github.com/onsi/gomega/gexec"
)

var _ = Describe("Coverage Specs", func() {
AfterEach(func() {
os.RemoveAll("./_fixtures/coverage_fixture/coverage_fixture.coverprofile")
})

It("runs coverage analysis in series and in parallel", func() {
session := startGinkgo("./_fixtures/coverage_fixture", "-cover")
Eventually(session).Should(gexec.Exit(0))
output := session.Out.Contents()
Ω(string(output)).Should(ContainSubstring("coverage: 80.0% of statements"))
Context("when it runs coverage analysis in series and in parallel", func() {
AfterEach(func() {
removeSuccessfully("./_fixtures/coverage_fixture/coverage_fixture.coverprofile")
})
It("works", func() {
session := startGinkgo("./_fixtures/coverage_fixture", "-cover")
Eventually(session).Should(gexec.Exit(0))

Ω(session.Out).Should(gbytes.Say(("coverage: 80.0% of statements")))

coverFile := "./_fixtures/coverage_fixture/coverage_fixture.coverprofile"
serialCoverProfileOutput, err := exec.Command("go", "tool", "cover", fmt.Sprintf("-func=%s", coverFile)).CombinedOutput()
Ω(err).ShouldNot(HaveOccurred())

serialCoverProfileOutput, err := exec.Command("go", "tool", "cover", "-func=./_fixtures/coverage_fixture/coverage_fixture.coverprofile").CombinedOutput()
Ω(err).ShouldNot(HaveOccurred())
removeSuccessfully(coverFile)

os.RemoveAll("./_fixtures/coverage_fixture/coverage_fixture.coverprofile")
Eventually(startGinkgo("./_fixtures/coverage_fixture", "-cover", "-nodes=4")).Should(gexec.Exit(0))

Eventually(startGinkgo("./_fixtures/coverage_fixture", "-cover", "-nodes=4")).Should(gexec.Exit(0))
parallelCoverProfileOutput, err := exec.Command("go", "tool", "cover", fmt.Sprintf("-func=%s", coverFile)).CombinedOutput()
Ω(err).ShouldNot(HaveOccurred())

parallelCoverProfileOutput, err := exec.Command("go", "tool", "cover", "-func=./_fixtures/coverage_fixture/coverage_fixture.coverprofile").CombinedOutput()
Ω(err).ShouldNot(HaveOccurred())
Ω(parallelCoverProfileOutput).Should(Equal(serialCoverProfileOutput))

Ω(parallelCoverProfileOutput).Should(Equal(serialCoverProfileOutput))
By("handling external packages", func() {
session = startGinkgo("./_fixtures/coverage_fixture", "-coverpkg=github.com/onsi/ginkgo/integration/_fixtures/coverage_fixture,github.com/onsi/ginkgo/integration/_fixtures/coverage_fixture/external_coverage_fixture")
Eventually(session).Should(gexec.Exit(0))

By("handling external packages")
session = startGinkgo("./_fixtures/coverage_fixture", "-coverpkg=github.com/onsi/ginkgo/integration/_fixtures/coverage_fixture,github.com/onsi/ginkgo/integration/_fixtures/coverage_fixture/external_coverage_fixture")
Eventually(session).Should(gexec.Exit(0))
output = session.Out.Contents()
Ω(output).Should(ContainSubstring("coverage: 71.4% of statements in github.com/onsi/ginkgo/integration/_fixtures/coverage_fixture, github.com/onsi/ginkgo/integration/_fixtures/coverage_fixture/external_coverage_fixture"))
Ω(session.Out).Should(gbytes.Say("coverage: 71.4% of statements in github.com/onsi/ginkgo/integration/_fixtures/coverage_fixture, github.com/onsi/ginkgo/integration/_fixtures/coverage_fixture/external_coverage_fixture"))

serialCoverProfileOutput, err = exec.Command("go", "tool", "cover", "-func=./_fixtures/coverage_fixture/coverage_fixture.coverprofile").CombinedOutput()
Ω(err).ShouldNot(HaveOccurred())
serialCoverProfileOutput, err = exec.Command("go", "tool", "cover", fmt.Sprintf("-func=%s", coverFile)).CombinedOutput()
Ω(err).ShouldNot(HaveOccurred())

os.RemoveAll("./_fixtures/coverage_fixture/coverage_fixture.coverprofile")
removeSuccessfully("./_fixtures/coverage_fixture/coverage_fixture.coverprofile")

Eventually(startGinkgo("./_fixtures/coverage_fixture", "-coverpkg=github.com/onsi/ginkgo/integration/_fixtures/coverage_fixture,github.com/onsi/ginkgo/integration/_fixtures/coverage_fixture/external_coverage_fixture", "-nodes=4")).Should(gexec.Exit(0))
Eventually(startGinkgo("./_fixtures/coverage_fixture", "-coverpkg=github.com/onsi/ginkgo/integration/_fixtures/coverage_fixture,github.com/onsi/ginkgo/integration/_fixtures/coverage_fixture/external_coverage_fixture", "-nodes=4")).Should(gexec.Exit(0))

parallelCoverProfileOutput, err = exec.Command("go", "tool", "cover", "-func=./_fixtures/coverage_fixture/coverage_fixture.coverprofile").CombinedOutput()
Ω(err).ShouldNot(HaveOccurred())
parallelCoverProfileOutput, err = exec.Command("go", "tool", "cover", fmt.Sprintf("-func=%s", coverFile)).CombinedOutput()
Ω(err).ShouldNot(HaveOccurred())

Ω(parallelCoverProfileOutput).Should(Equal(serialCoverProfileOutput))
Ω(parallelCoverProfileOutput).Should(Equal(serialCoverProfileOutput))
})
})
})

It("validates coverprofile sets custom profile name", func() {
session := startGinkgo("./_fixtures/coverage_fixture", "-cover", "-coverprofile=coverage.txt")

Eventually(session).Should(gexec.Exit(0))
Context("when a custom profile name is specified", func() {
AfterEach(func() {
removeSuccessfully("./_fixtures/coverage_fixture/coverage.txt")
})

// Check that the correct file was created
_, err := os.Stat("./_fixtures/coverage_fixture/coverage.txt")
It("generates cover profiles with the specified name", func() {
session := startGinkgo("./_fixtures/coverage_fixture", "-cover", "-coverprofile=coverage.txt")
Eventually(session).Should(gexec.Exit(0))

Ω(err).ShouldNot(HaveOccurred())

// Cleanup
os.RemoveAll("./_fixtures/coverage_fixture/coverage.txt")
Ω("./_fixtures/coverage_fixture/coverage.txt").Should(BeARegularFile())
})
})

It("Works in recursive mode", func() {
session := startGinkgo("./_fixtures/combined_coverage_fixture", "-r", "-cover", "-coverprofile=coverage.txt")

Eventually(session).Should(gexec.Exit(0))

packages := []string{"first_package", "second_package"}
Context("when run in recursive mode", func() {
AfterEach(func() {
removeSuccessfully("./_fixtures/combined_coverage_fixture/coverage-recursive.txt")
removeSuccessfully("./_fixtures/combined_coverage_fixture/first_package/coverage-recursive.txt")
removeSuccessfully("./_fixtures/combined_coverage_fixture/second_package/coverage-recursive.txt")
})

for _, p := range packages {
coverFile := fmt.Sprintf("./_fixtures/combined_coverage_fixture/%s/coverage.txt", p)
_, err := os.Stat(coverFile)
It("generates a coverage file per package", func() {
session := startGinkgo("./_fixtures/combined_coverage_fixture", "-r", "-cover", "-coverprofile=coverage-recursive.txt")
Eventually(session).Should(gexec.Exit(0))

Ω(err).ShouldNot(HaveOccurred())

// Cleanup
os.RemoveAll(coverFile)
}
Ω("./_fixtures/combined_coverage_fixture/first_package/coverage-recursive.txt").Should(BeARegularFile())
Ω("./_fixtures/combined_coverage_fixture/second_package/coverage-recursive.txt").Should(BeARegularFile())
})
})

It("Works in parallel mode", func() {
session := startGinkgo("./_fixtures/coverage_fixture", "-p", "-cover", "-coverprofile=coverage.txt")

Eventually(session).Should(gexec.Exit(0))
Context("when run in parallel mode", func() {
AfterEach(func() {
removeSuccessfully("./_fixtures/coverage_fixture/coverage-parallel.txt")
})

coverFile := "./_fixtures/coverage_fixture/coverage.txt"
_, err := os.Stat(coverFile)
It("works", func() {
session := startGinkgo("./_fixtures/coverage_fixture", "-p", "-cover", "-coverprofile=coverage-parallel.txt")

Ω(err).ShouldNot(HaveOccurred())
Eventually(session).Should(gexec.Exit(0))

// Cleanup
os.RemoveAll(coverFile)
Ω("./_fixtures/coverage_fixture/coverage-parallel.txt").Should(BeARegularFile())
})
})

It("Appends coverages if output dir and coverprofile were set", func() {
session := startGinkgo("./_fixtures/combined_coverage_fixture", "-outputdir=./", "-r", "-cover", "-coverprofile=coverage.txt")

Eventually(session).Should(gexec.Exit(0))

_, err := os.Stat("./_fixtures/combined_coverage_fixture/coverage.txt")

Ω(err).ShouldNot(HaveOccurred())

// Cleanup
os.RemoveAll("./_fixtures/combined_coverage_fixture/coverage.txt")
Context("when run in recursive mode specifying a coverprofile", func() {
AfterEach(func() {
removeSuccessfully("./_fixtures/combined_coverage_fixture/coverprofile-recursive.txt")
removeSuccessfully("./_fixtures/combined_coverage_fixture/first_package/coverprofile-recursive.txt")
removeSuccessfully("./_fixtures/combined_coverage_fixture/second_package/coverprofile-recursive.txt")
})

It("combines the coverages", func() {
session := startGinkgo("./_fixtures/combined_coverage_fixture", "-outputdir=./", "-r", "-cover", "-coverprofile=coverprofile-recursive.txt")
Eventually(session).Should(gexec.Exit(0))

By("generating a combined coverage file", func() {
Ω("./_fixtures/combined_coverage_fixture/coverprofile-recursive.txt").Should(BeARegularFile())
})

By("also generating the single package coverage files", func() {
Ω("./_fixtures/combined_coverage_fixture/first_package/coverprofile-recursive.txt").Should(BeARegularFile())
Ω("./_fixtures/combined_coverage_fixture/second_package/coverprofile-recursive.txt").Should(BeARegularFile())
})
})
})

It("Fails with an error if output dir and coverprofile were set, but the output dir did not exist", func() {
Expand All @@ -120,28 +129,19 @@ var _ = Describe("Coverage Specs", func() {
Ω(string(output)).Should(ContainSubstring("Unable to create combined profile, outputdir does not exist: ./all/profiles/here"))
})

It("Moves coverages if only output dir was set", func() {
session := startGinkgo("./_fixtures/combined_coverage_fixture", "-outputdir=./", "-r", "-cover")

Eventually(session).Should(gexec.Exit(0))

packages := []string{"first_package", "second_package"}

for _, p := range packages {
coverFile := fmt.Sprintf("./_fixtures/combined_coverage_fixture/%s.coverprofile", p)

// Cleanup
defer func(f string) {
os.RemoveAll(f)
}(coverFile)

defer func(f string) {
os.RemoveAll(fmt.Sprintf("./_fixtures/combined_coverage_fixture/%s/coverage.txt", f))
}(p)

_, err := os.Stat(coverFile)

Ω(err).ShouldNot(HaveOccurred())
}
Context("when only output dir was set", func() {
AfterEach(func() {
removeSuccessfully("./_fixtures/combined_coverage_fixture/first_package.coverprofile")
removeSuccessfully("./_fixtures/combined_coverage_fixture/first_package/coverage.txt")
removeSuccessfully("./_fixtures/combined_coverage_fixture/second_package.coverprofile")
removeSuccessfully("./_fixtures/combined_coverage_fixture/second_package/coverage.txt")
})
It("moves coverages", func() {
session := startGinkgo("./_fixtures/combined_coverage_fixture", "-outputdir=./", "-r", "-cover")
Eventually(session).Should(gexec.Exit(0))

Ω("./_fixtures/combined_coverage_fixture/first_package.coverprofile").Should(BeARegularFile())
Ω("./_fixtures/combined_coverage_fixture/second_package.coverprofile").Should(BeARegularFile())
})
})
})
5 changes: 5 additions & 0 deletions integration/integration_suite_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,3 +89,8 @@ func startGinkgo(dir string, args ...string) *gexec.Session {
Ω(err).ShouldNot(HaveOccurred())
return session
}

func removeSuccessfully(path string) {

@williammartin williammartin Feb 13, 2018

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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?

@alamages alamages Feb 14, 2018

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

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.

err := os.RemoveAll(path)
Expect(err).NotTo(HaveOccurred())
}