test: add code coverage support #550
Conversation
rchincha
force-pushed
the
cov-new
branch
2 times, most recently
from
c4ac426 to
e106109
Compare
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #550 +/- ##
===========================================
+ Coverage 13.01% 54.33% +41.32%
===========================================
Files 40 64 +24
Lines 6003 7479 +1476
===========================================
+ Hits 781 4064 +3283
+ Misses 5094 2724 -2370
- Partials 128 691 +563 ☔ View full report in Codecov by Sentry. |
rchincha
force-pushed
the
cov-new
branch
5 times, most recently
from
b029e55 to
81de581
Compare
There was a problem hiding this comment.
this looks really good, thank you. It will be really great to get "coverage credit" for the integration tests.
two overall comments:
- Would it be simpler to just build both static-stacker and stacker-cov in the same stacker.yaml invocation? it messes up make dependencies a bit, but you can kind of just say "oh well" as stacker-cov always gets built when static-stacker is built.
- Lets put the gocov into /stacker/working or something. https://github.com/project-stacker/stacker/pull/543/files does a reasonable job of putting all our work under a single point inside.
unless there is some reason to not run integration adn unit tests with coverage, I really think we should just do that all the time.
rchincha
force-pushed
the
cov-new
branch
12 times, most recently
from
0cbfd6c to
4177c91
Compare
|
Also, doesn't make sense to trust integration testing on an instrumented non-production binary - these should be kept separate. |
There was a problem hiding this comment.
I have lots of comments on this, but none of them compare to a "+40% coverage" change.
I'll just put them here though for my later reference.
- I don't think a separate "coverage" workflow is useful. c-i should always run coverage. Im' not sure what caching we get between the 'coverage' and 'build' workflow, which means just doing lots of extra things. While they do seem to happen in parallel, that must multiplies the transient failure rate by 2.
- i think there are cleanups to be had in the makefile or stacker file changes. we can do a better job there.
again, none of this comes near the huge win of getting coverage on integration tests. thank you.
pkg/container/userns.go
Outdated
| @@ -33,6 +33,12 @@ func MaybeRunInNamespace(config types.StackerConfig, userCmd []string) error { | |||
| euid := os.Geteuid() | |||
| env = append(env, fmt.Sprintf("%s=%d", envName, euid)) | |||
|
|
|||
| /* | |||
With golang 1.20.x, we can build a binary with coverage support.
"
Cover
Go 1.20 supports collecting code coverage profiles for programs
(applications and integration tests), as opposed to just unit tests.
To collect coverage data for a program, build it with go build's
-cover
flag, then run the resulting binary with the environment variable
GOCOVERDIR set to an output directory for coverage profiles. See the
'coverage for integration tests' landing page for more on how to get
started. For details on the design and implementation, see the
proposal.
"
Signed-off-by: Ramkumar Chinchani <rchincha@cisco.com>
^ it is separate because binaries being built and shipped are separate (unfortunately) - we want the binary from one workflow and coverage from the other. But both must be independently validated for different reasons. |
Just build them both from the same run. A 'make' should build both. One is named 'stacker-coverage' one is named 'stacker'. I can do a PR with my suggested cleanups at some point. |
With golang 1.20.x, we can build a binary with coverage support.
What type of PR is this?
Which issue does this PR fix:
What does this PR do / Why do we need it:
If an issue # is not available please add repro steps and logs showing the issue:
Testing done on this change:
Automation added to e2e:
Will this break upgrades or downgrades?
Does this PR introduce any user-facing change?:
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.