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

Paired upgrade tests #4095

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

mgencur
Copy link
Contributor

@mgencur mgencur commented Sep 11, 2024

Fixes #4047

Proposed Changes

  • The upgrade tests now test every feature in 4 different scenarios:
    • Run the same test post-upgrade and post-downgrade. Create resources and verify them at the same phase.
    • Create resources pre-upgrade and verified/removed them post-upgrade.
    • Create resources pre-upgrade, verify post-upgrade, verify and remove post-downgrade.
    • Create resources post-upgrade, verify and remove post-downgrade.
  • Re-use REKT-based tests for upgrade testing
  • Sink-specific tests were removed because they're now covered by the KafkaSinkSourceBinaryEventFeature and KafkaSinkSourceStructuredEventFeature. This feature uses both KafkaSource and KafkaSink
  • KafkaChannel tests now run only with binary encoding as it uses ContainerSource that is based on the heartbeats image. Previously they were running with both but now it's re-using a test that uses just binary encoding. I don't think testing binary vs. structured encoding in upgrade tests is really necessary.
  • Speed up test execution of the whole upgrade test suite. There were a couple of successful runs with this new approach that took 34m24s, 33m1s. Looking at the old approach and successful runs, there were: 38m50s, 40m9s, 38m52s, 43m47s, 42m1s. The new approach is about 6-7 minutes faster on average than the previous approach.

Release Note


Docs

@knative-prow knative-prow bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 11, 2024
Copy link

knative-prow bot commented Sep 11, 2024

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: mgencur
Once this PR has been reviewed and has the lgtm label, please assign creydr for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@knative-prow knative-prow bot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Sep 11, 2024
@knative-prow knative-prow bot added area/test size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Sep 11, 2024
Copy link

codecov bot commented Sep 11, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 48.44%. Comparing base (6c4d180) to head (cfa3781).

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #4095   +/-   ##
=======================================
  Coverage   48.44%   48.44%           
=======================================
  Files         244      244           
  Lines       14876    14876           
=======================================
  Hits         7207     7207           
  Misses       6942     6942           
  Partials      727      727           

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

@mgencur
Copy link
Contributor Author

mgencur commented Sep 12, 2024

The upgrade tests show a single failure now: https://prow.knative.dev/view/gs/knative-prow/pr-logs/pull/knative-extensions_eventing-kafka-broker/4095/upgrade-tests_eventing-kafka-broker_main/1834210401064062976

Trying again to see if this is reproducible.

/test upgrade-tests

@pierDipi
Copy link
Member

pierDipi commented Sep 12, 2024

E0912 12:47:54.384853   42406 library.go:117] Package knative.dev/eventing/test/upgrade does not have module info. Non go modules projects are no longer supported. For feedback, refer to https://github.com/google/go-licenses/issues/128.
F0912 12:47:55.359532   42406 main.go:77] some errors occurred when loading direct and transitive dependency packages
exit status 1
--- FAIL: go-licenses failed the license check}

@mgencur
Copy link
Contributor Author

mgencur commented Sep 12, 2024

This PR requires changes from knative/eventing#8190 . When it's merged I will update dependencies and this failure should go away.
I'm going to provide issue description tomorrow - with details about what's changed. The CI runtime looks better than before, and test coverage is increased!

@knative-prow-robot knative-prow-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 20, 2024
@knative-prow-robot knative-prow-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 20, 2024
@mgencur mgencur changed the title [WIP] Paired upgrade tests Paired upgrade tests Sep 20, 2024
@knative-prow knative-prow bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 20, 2024
@mgencur
Copy link
Contributor Author

mgencur commented Sep 20, 2024

This is ready for reviews.

@mgencur
Copy link
Contributor Author

mgencur commented Sep 20, 2024

The reconciler-tests-namespaced-broker_eventing-kafka-broker_main fails with same failures as on other PRs. Not related to my PR.

@mgencur
Copy link
Contributor Author

mgencur commented Sep 20, 2024

/retest

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/data-plane area/test size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
3 participants