Skip to content
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

Jump to bottom

test: enable coverage of crit unit/e2e tests #121

Merged
merged 5 commits into from
Apr 16, 2023
Merged

test: enable coverage of crit unit/e2e tests #121

merged 5 commits into from
Apr 16, 2023

Conversation

rst0git
Copy link
Member

@rst0git rst0git commented Apr 15, 2023 •
edited
Loading

This pull request enables code coverage for CRIT's unit and e2e tests.

@codecov
Copy link

codecov bot commented Apr 15, 2023 •
edited
Loading

Codecov Report

Patch coverage has no change and project coverage change: +3.69 🎉

Comparison is base (06557f1) 18.72% compared to head (dbf1473) 22.42%.

Additional details and impacted files 
@@            Coverage Diff             @@
##           master     #121      +/-   ##
==========================================
+ Coverage   18.72%   22.42%   +3.69%     
==========================================
  Files          93       94       +1     
  Lines       15081    15207     +126     
==========================================
+ Hits         2824     3410     +586     
+ Misses      11979    11501     -478     
- Partials      278      296      +18

see 24 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@rst0git rst0git marked this pull request as draft April 15, 2023 12:28
@rst0git rst0git marked this pull request as ready for review April 15, 2023 12:54
@rst0git rst0git requested a review from adrianreber April 15, 2023 12:54
@rst0git rst0git force-pushed the crit-unit-tests branch 3 times, most recently from 622b1d7 to 3277afa Compare April 15, 2023 14:03
@rst0git rst0git changed the title test: enable coverage of crit unit tests test: enable coverage of crit unit/e2e tests Apr 15, 2023
@rst0git
Copy link
Member Author

rst0git commented Apr 15, 2023

@snprajwal PTAL

@adrianreber
Copy link
Member

Nice change. Moving the unit test to crit/ sounds like the right thing to do.

@adrianreber
Copy link
Member 

go test  -v ./...
=== RUN   TestGetDumpStats
--- PASS: TestGetDumpStats (0.00s)
=== RUN   TestGetRestoreStats
--- PASS: TestGetRestoreStats (0.00s)
PASS
ok  	github.com/checkpoint-restore/go-criu/v6/crit	0.004s
?   	github.com/checkpoint-restore/go-criu/v6/crit/cmd	[no test files]
?   	github.com/checkpoint-restore/go-criu/v6/crit/images	[no test files]

This is also a good indicator that we should add some more tests.

@snprajwal
Copy link
Member

snprajwal commented Apr 15, 2023 •
edited
Loading

@adrianreber crit/images and crit/cmd are being refactored in #109, I'll add the tests there for the parts we currently aren't testing

@adrianreber
Copy link
Member

@adrianreber crit/images and crit/cmd are being refactored in the other PR, I'll add the tests there for the parts we currently aren't testing

Sounds good.

@adrianreber
Copy link
Member

One thing I am thinking about is that for unit tests we could always generate test coverage output while using go test. Just to reduce code in the Makefiles. @rst0git what do you think? We can also merge what we have in this PR. Works both for me.

@snprajwal
Copy link
Member

snprajwal commented Apr 15, 2023 •
edited
Loading

We can have a unit-test target in the top level Makefile (or the Makefile in test) which simply runs
go -coverprofile=$COVER_PROFILE_PATH/coverprofile-unit-test test ./... in the root.

@adrianreber
Copy link
Member

We can have a unit-test target in the top level Makefile (or the Makefile in test) which simply runs go -coverprofile=$COVER_PROFILE_PATH/coverprofile-unit-test test ./... in the root.

Sounds like a good idea.

@rst0git rst0git force-pushed the crit-unit-tests branch 3 times, most recently from 433385f to bb4df14 Compare April 15, 2023 23:17
@rst0git rst0git requested a review from adrianreber April 15, 2023 23:21
snprajwal
snprajwal approved these changes Apr 16, 2023
View reviewed changes
Copy link
Member

@snprajwal snprajwal left a comment

Choose a reason for hiding this comment

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

LGTM

@adrianreber
Copy link
Member

@rst0git works for me. There still seems to be one question open. What about always running unit tests with test coverage results? Unit test coverage does not depend on Go 1.20 and can always be created.

@rst0git
Copy link
Member Author

rst0git commented Apr 16, 2023

What about always running unit tests with test coverage results? Unit test coverage does not depend on Go 1.20 and can always be created.

I have added a commit to always run the unit tests with test coverage results.

@rst0git
crit: move unit test to crit/ pkg
91569bf 
To generate code coverage for the unit tests of crit they need to be
part of the crit package. This patch moves the unit test from
'test/crit/' to 'crit/' as a preparation for a subsequent patch where
we would enable test coverage.

Signed-off-by: Radostin Stoyanov <[email protected]>
rst0git added 4 commits April 16, 2023 17:07
@rst0git
test: enable coverage of crit unit tests
54f0428 
This patch enables test coverage for the unit tests of crit.

Suggested-by: Adrian Reber <[email protected]>
Signed-off-by: Radostin Stoyanov <[email protected]>
@rst0git
test: enable coverage of crit e2e tests
7400d2e 
This patch enables coverage for the end-to-end tests of crit.

Suggested-by: Adrian Reber <[email protected]>
Signed-off-by: Radostin Stoyanov <[email protected]>
@rst0git
test: add makefile for test/loop
36808fe 
To reduce code duplication, we add a Makefile for building the
`loop` executable.

Signed-off-by: Radostin Stoyanov <[email protected]>
@rst0git
crit: run unit tests with coverage results
dbf1473 
There are two ways we can enable test coverage for the unit tests:
1) `-coverprofile` and 2) `-cover`. The first method stores the results
in a file that we can upload with `codecov`. The second method only
outputs the test coverage results to standard output.

This patch set the default value of GOFLAGS to `-cover` to
always show the coverage of unit tests.

Suggested-by: Adrian Reber <[email protected]>
Signed-off-by: Radostin Stoyanov <[email protected]>
@adrianreber adrianreber merged commit 8a38048 into checkpoint-restore:master Apr 16, 2023
@rst0git rst0git deleted the crit-unit-tests branch April 16, 2023 16:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@snprajwal snprajwal snprajwal approved these changes

@adrianreber adrianreber adrianreber approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@rst0git @adrianreber @snprajwal