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.

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

Revamping rekor e2e - part 1 of N #1089

Merged
merged 8 commits into from
Oct 25, 2022

Conversation

naveensrinivasan
Copy link
Contributor

Summary

Refactoring e2e tests.
There will be multiple PRs to move the e2e to specific folders. Some code is duplicated up until all the e2e code is refactored.
#1075

Release Note

NONE

Documentation

NONE

@naveensrinivasan naveensrinivasan requested review from cpanato and a team as code owners October 3, 2022 02:31
@bobcallaway
Copy link
Member

Does this still generate appropriate code coverage after #1071 @cdris ?

Copy link

@cdris cdris left a comment

Choose a reason for hiding this comment

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

Hello! As Bob mentioned, I recently updated the e2e tests to create code coverage reports in #1071. I've commented what needs to change below to support code coverage. Once util.go and e2e-test.sh are updated, all of the moving of tests going forward should be fine as long as they're using all the same util run functions.

pkg/e2e-test.sh Outdated Show resolved Hide resolved
pkg/e2e-test.sh Outdated Show resolved Hide resolved
pkg/e2e-test.sh Show resolved Hide resolved
.github/workflows/main.yml Show resolved Hide resolved
pkg/util/util.go Show resolved Hide resolved
echo
echo "running tests"
REKORTMPDIR="$(mktemp -d -t rekor_test.XXXXXX)"
cp $dir/rekor-cli $REKORTMPDIR/rekor-cli
Copy link

Choose a reason for hiding this comment

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

Is there a reason you're copying the cli instead of calling it with ../ in util.go?

@codecov-commenter
Copy link

codecov-commenter commented Oct 11, 2022

Codecov Report

Merging #1089 (741e675) into main (3942a0e) will increase coverage by 15.23%.
The diff coverage is n/a.

@@             Coverage Diff             @@
##             main    #1089       +/-   ##
===========================================
+ Coverage   48.89%   64.13%   +15.23%     
===========================================
  Files          82       82               
  Lines        7620     7620               
===========================================
+ Hits         3726     4887     +1161     
+ Misses       3194     2109     -1085     
+ Partials      700      624       -76     
Flag Coverage Δ
e2etests 48.92% <ø> (+0.02%) ⬆️
unittests 41.56% <ø> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
pkg/types/entries.go 47.45% <0.00%> (+1.69%) ⬆️
pkg/api/index.go 71.17% <0.00%> (+1.80%) ⬆️
pkg/pki/factory.go 83.33% <0.00%> (+1.85%) ⬆️
cmd/rekor-cli/app/search.go 57.85% <0.00%> (+2.14%) ⬆️
pkg/types/jar/v0.0.1/entry.go 57.76% <0.00%> (+2.91%) ⬆️
pkg/util/validate.go 100.00% <0.00%> (+3.92%) ⬆️
pkg/pki/ssh/encode.go 54.71% <0.00%> (+5.66%) ⬆️
pkg/types/tuf/v0.0.1/entry.go 62.04% <0.00%> (+9.38%) ⬆️
pkg/types/rekord/v0.0.1/entry.go 67.76% <0.00%> (+9.53%) ⬆️
pkg/types/intoto/v0.0.2/entry.go 70.31% <0.00%> (+9.89%) ⬆️
... and 29 more

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

@naveensrinivasan naveensrinivasan requested review from cdris and removed request for cpanato October 12, 2022 00:29
Copy link

@cdris cdris left a comment

Choose a reason for hiding this comment

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

Just need to update all of the /tmp/rekor references to /tmp/pkg-rekor so the code coverage doesn't overlap with the existing e2e tests. Also need to actually add the coverage flag to args before calling the cli in tests.

pkg/util/util.go Show resolved Hide resolved
pkg/util/util.go Show resolved Hide resolved
pkg/e2e-test.sh Outdated Show resolved Hide resolved
pkg/e2e-test.sh Outdated Show resolved Hide resolved
- Refactor e2e tests to move it to specific folders.

Signed-off-by: naveensrinivasan <172697+naveensrinivasan@users.noreply.github.com>
Signed-off-by: naveensrinivasan <172697+naveensrinivasan@users.noreply.github.com>
Signed-off-by: naveensrinivasan <172697+naveensrinivasan@users.noreply.github.com>
Signed-off-by: naveensrinivasan <172697+naveensrinivasan@users.noreply.github.com>
Signed-off-by: naveensrinivasan <172697+naveensrinivasan@users.noreply.github.com>
pkg/util/util.go Outdated Show resolved Hide resolved
pkg/util/util.go Outdated Show resolved Hide resolved
Signed-off-by: naveensrinivasan <172697+naveensrinivasan@users.noreply.github.com>
cdris
cdris previously approved these changes Oct 21, 2022
Copy link

@cdris cdris left a comment

Choose a reason for hiding this comment

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

lgtm

@naveensrinivasan
Copy link
Contributor Author

lgtm

Thanks

@naveensrinivasan
Copy link
Contributor Author

@sigstore/rekor-codeowners A friendly reminder!

Copy link
Member

@bobcallaway bobcallaway left a comment

Choose a reason for hiding this comment

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

one question and two nits

pkg/e2e-test.sh Outdated Show resolved Hide resolved
pkg/util/util.go Outdated Show resolved Hide resolved
pkg/e2e-test.sh Outdated Show resolved Hide resolved
Signed-off-by: naveensrinivasan <172697+naveensrinivasan@users.noreply.github.com>
Comment on lines +64 to +66
echo "generating code coverage"
curl -X GET 0.0.0.0:2345/kill
sleep 5
Copy link
Member

Choose a reason for hiding this comment

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

Something we need to consider, is that part of the e2e tests have dependencies on specific entries; the Rekor instance is launched without specifying a particular TreeID (for these tests), and therefore as you break this apart you will need to think through which entries a test needs in order to be successful.

Copy link
Member

Choose a reason for hiding this comment

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

Since we kill the instance to gather code coverage here, the entries are lost unless in the future we change these tests to start Rekor targeted at a specific treeID.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So should we populate the rekor with specific entries before the tests? How is it working for the existing e2e? Is this a missing feature at present?

Copy link
Member

Choose a reason for hiding this comment

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

At the moment, this is more of an FYI rather than something that's directly actionable. Just something for you to be aware of as you continue moving things over, is that unless we change the test harness to be more shard aware, you may run into issues around tests that are dependent on one another. We should probably make those explicit (with wrapping and use of t.Run) whenever we see them.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, Thanks. What else is required to get this merged in?

Copy link
Member

Choose a reason for hiding this comment

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

tests need to pass (I just re-triggered them).

Signed-off-by: Naveen <172697+naveensrinivasan@users.noreply.github.com>
@bobcallaway bobcallaway merged commit 6551890 into sigstore:main Oct 25, 2022
@github-actions github-actions bot added this to the v1.1.0 milestone Oct 25, 2022
@naveensrinivasan naveensrinivasan deleted the naveen/refactor-e2e branch October 25, 2022 00:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants