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

Convert legacy docker tests from bash to golang #11357

Merged
merged 14 commits into from
Nov 26, 2024

Conversation

dereknola
Copy link
Member

@dereknola dereknola commented Nov 22, 2024

Background

The current docker tests use a complex series of bash function hooks to construct k3s cluster out of docker containers, using the rancher/k3s image we publish. These tests are difficult to read, difficult to write, and do not match the ginkgo/gomega centric BDD testing frameworks we have constructed for integration and E2E tests.

Proposed Changes

  • Convert the old docker tests to use Ginkgo/Gomega BDD testing framework. This framework is currently similar but separate from E2E and Integration frameworks. Future work is to potentially combine all 3 frameworks where it makes sense.
  • Precompile Go tests binaries during a separate build job to speed of the actual running of the tests. This runs in parallel with the Building of the k3s binary and image, which is the current bottleneck.
  • compat test is now skew test, as we are testing the skew policy of K8s.

Note: For now this is going to exist alongside the legacy docker tests. More work needs to be done to ensure things like log output and failures are handled effectively.

Types of Changes

Major CI change, Testing

Verification

https://github.com/k3s-io/k3s/actions/runs/12019291772
image

Testing

Its all new tests!

Linked Issues

TBD

User-Facing Change


Further Comments

Prebuild K3s Go Tests

Strip go test binaries to reduce size

Crank native GHA compression

Signed-off-by: Derek Nola <[email protected]>
Signed-off-by: Derek Nola <[email protected]>
Signed-off-by: Derek Nola <[email protected]>
Signed-off-by: Derek Nola <[email protected]>
Signed-off-by: Derek Nola <[email protected]>
Copy link

codecov bot commented Nov 22, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 38.05%. Comparing base (cd4dded) to head (5301b89).
Report is 8 commits behind head on master.

❗ There is a different number of reports uploaded between BASE (cd4dded) and HEAD (5301b89). Click for more details.

HEAD has 2 uploads less than BASE
Flag BASE (cd4dded) HEAD (5301b89)
unittests 1 0
e2etests 7 6
Additional details and impacted files
@@            Coverage Diff             @@
##           master   #11357      +/-   ##
==========================================
- Coverage   47.18%   38.05%   -9.14%     
==========================================
  Files         179      162      -17     
  Lines       18600    18010     -590     
==========================================
- Hits         8776     6853    -1923     
- Misses       8469     9959    +1490     
+ Partials     1355     1198     -157     
Flag Coverage Δ
e2etests 33.95% <ø> (-8.26%) ⬇️
inttests 34.71% <ø> (+0.02%) ⬆️
unittests ?

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@dereknola dereknola marked this pull request as ready for review November 25, 2024 17:26
@dereknola dereknola requested a review from a team as a code owner November 25, 2024 17:26
Copy link
Member

@brandond brandond left a comment

Choose a reason for hiding this comment

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

one small nit, LGTM otherwise!

tests/docker/basics/basics_test.go Outdated Show resolved Hide resolved
tests/docker/test-helpers.go Show resolved Hide resolved
Comment on lines +391 to +392
// TODO the below functions are duplicated in the integration test utils. Consider combining into commmon package
// DeploymentsReady checks if the provided list of deployments are ready, otherwise returns an error
Copy link
Member

Choose a reason for hiding this comment

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

👍🏻 on combining these as soon as possible.

We also have another copy in rke2. Maybe we need a common repo for the test framework. Maybe there already is one from k8s and we should be using that instead.

Signed-off-by: Derek Nola <[email protected]>
brandond
brandond previously approved these changes Nov 25, 2024
manuelbuil
manuelbuil previously approved these changes Nov 26, 2024
Copy link
Contributor

@manuelbuil manuelbuil left a comment

Choose a reason for hiding this comment

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

Just one small wrong comment.

Future work is to potentially combine all 3 frameworks where it makes sense.

That would be awesome. It feels like we are testing some stuff three times :)

tests/docker/test-helpers.go Outdated Show resolved Hide resolved
spec:
containers:
- name: stargz-snapshot-test
image: "ghcr.io/stargz-containers/k3s-test-ubuntu:20.04-esgz"
Copy link
Contributor

Choose a reason for hiding this comment

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

ubuntu:20.04 oh no, so old

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah there is future work to potentially:

  • Update this image
  • No longer test this functionality, IDK how many people are still using this.
  • Move to a newer test image that supports the same thing from someone else.

Signed-off-by: Derek Nola <[email protected]>
@dereknola dereknola dismissed stale reviews from manuelbuil and brandond via 5301b89 November 26, 2024 18:55
Copy link
Member

@brandond brandond left a comment

Choose a reason for hiding this comment

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

Maybe squash when merging?

@dereknola
Copy link
Member Author

Yeah I'm gonna squash

@dereknola dereknola merged commit b5e2fa7 into k3s-io:master Nov 26, 2024
37 checks passed
@dereknola dereknola deleted the k3s_docker_compose branch November 26, 2024 20:31
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.

3 participants