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

fix: knative ingress tests #1646

Merged
merged 5 commits into from
Aug 4, 2021
Merged

fix: knative ingress tests #1646

merged 5 commits into from
Aug 4, 2021

Conversation

shaneutt
Copy link
Contributor

@shaneutt shaneutt commented Aug 3, 2021

What this PR does / why we need it:

When releasing v2.0.0-alpha.3 some problems were encountered with the Knative ingress tests, which turned out to be flakes caused by timing issues with deploying Knative components. This PR removes all provisioning of Knative components in favor of leaving that up to the testing environment's Knative addon, fixes some issues, does some refactoring of the tests and ultimately re-enables the tests in release testing CI in support of upcoming v2.x releases.

Which issue this PR fixes

Fixes #1637

Special notes for your reviewer:

Blocked by Kong/kubernetes-testing-framework#77

I have tested this manually with both v1.17 and v1.18 GKE clusters locally to to ensure they pass. I made sure to run the tests multiple times in a row to validate cleanup and ensure test idempotency, and did not run into any errors.

@shaneutt shaneutt added bug Something isn't working blocked priority/medium labels Aug 3, 2021
@shaneutt shaneutt self-assigned this Aug 3, 2021
@shaneutt shaneutt added the do not merge let the author merge this, don't merge for them. label Aug 3, 2021
@shaneutt shaneutt marked this pull request as ready for review August 3, 2021 20:10
@shaneutt shaneutt requested a review from a team as a code owner August 3, 2021 20:10
@codecov
Copy link

codecov bot commented Aug 3, 2021

Codecov Report

Merging #1646 (1ae6d8e) into next (de8cb96) will increase coverage by 0.15%.
The diff coverage is n/a.

❗ Current head 1ae6d8e differs from pull request most recent head d690d7c. Consider uploading reports for the commit d690d7c to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##             next    #1646      +/-   ##
==========================================
+ Coverage   62.19%   62.35%   +0.15%     
==========================================
  Files          83       83              
  Lines        7283     7283              
==========================================
+ Hits         4530     4541      +11     
+ Misses       2421     2409      -12     
- Partials      332      333       +1     
Flag Coverage Δ
integration-test 62.35% <ø> (+0.15%) ⬆️

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

Impacted Files Coverage Δ
internal/ctrlutils/utils.go 86.53% <0.00%> (-1.93%) ⬇️
internal/ctrlutils/ingress-status.go 52.24% <0.00%> (-1.06%) ⬇️
internal/store/store.go 66.59% <0.00%> (+0.86%) ⬆️
internal/parser/parser.go 88.82% <0.00%> (+1.14%) ⬆️
...trollers/configuration/zz_generated_controllers.go 35.31% <0.00%> (+1.35%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5aa12a4...d690d7c. Read the comment docs.

Copy link
Contributor

@rainest rainest left a comment

Choose a reason for hiding this comment

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

Approved pending the post-blocker module changes.

This was causing a conflict under some conditions as the
CRDs could be loaded multiple times simoltaneously.
This patch fixes several issues with the knative tests
and adjusts them for the inclusion of the knative addon
from the test framework:

- removed deployment logic (the addon does this now)
- added resource cleanup for knative.Services created
- general code cleanup and logging improvements

The result is that tests are now passing consistently
against GKE clusters (particularly older v1.17 and v1.18
clusters) and can be run repeatedly without triggering
conflicts.
@shaneutt shaneutt enabled auto-merge (rebase) August 4, 2021 14:44
@shaneutt shaneutt merged commit f79e8ee into next Aug 4, 2021
@shaneutt shaneutt deleted the fix-knative-ingress-tests branch August 4, 2021 14:57
@shaneutt shaneutt linked an issue Aug 4, 2021 that may be closed by this pull request
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
blocked bug Something isn't working ci/license/unchanged do not merge let the author merge this, don't merge for them. priority/medium
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Knative integration tests not working on v1.17, v1.18
2 participants