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

test/e2e: verify resource status upon creation #6347

Merged
merged 3 commits into from Apr 30, 2024

Conversation

skriss
Copy link
Member

@skriss skriss commented Apr 15, 2024

Adds checks to verify resources are eventually
created with the expected status, where missing.
Without these checks, it's unknown whether the
"CreateAndWaitFor" method was successful or timed
out waiting for the desired condition.

@skriss skriss added the release-note/none-required Marks a PR as not requiring a release note. Should only be used for very small changes. label Apr 15, 2024
@skriss skriss requested a review from a team as a code owner April 15, 2024 21:03
@skriss skriss requested review from tsaarni and sunjayBhatia and removed request for a team April 15, 2024 21:03
@sunjayBhatia sunjayBhatia requested review from a team, rajatvig and davinci26 and removed request for a team April 15, 2024 21:03
Copy link

codecov bot commented Apr 15, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 81.59%. Comparing base (3c79035) to head (492416e).

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #6347   +/-   ##
=======================================
  Coverage   81.59%   81.59%           
=======================================
  Files         133      133           
  Lines       15861    15861           
=======================================
  Hits        12941    12941           
  Misses       2624     2624           
  Partials      296      296           

@skriss
Copy link
Member Author

skriss commented Apr 15, 2024

Considering nearly every invocation of the "CreateAndWaitFor" methods ignore the first returned argument, I'm going to look at dropping it from the API. Tests that need to re-fetch the Kube resource from the apiserver after creation can do it explicitly. Means all of these can become one-liners.

@@ -668,17 +658,16 @@ var _ = Describe("Gateway provisioner", func() {
} else {
// Root proxy in non-watched namespace should fail
By(fmt.Sprintf("Expect namespace %s not to be watched by contour", t.namespace))
hr, ok := f.CreateHTTPRouteAndWaitFor(route, e2e.HTTPRouteIgnoredByContour)
require.True(f.T(), ok, fmt.Sprintf("httproute's is %v", hr))
Copy link
Contributor

Choose a reason for hiding this comment

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

I did use it here for debugging purpose in case it fails.

But yeah it's doable just to add a function to get the resource, since most of the code base doesn't use this.

Adds checks to verify resources are eventually
created with the expected status, where missing.
Without these checks, it's unknown whether the
"CreateAndWaitFor" method was successful or timed
out waiting for the desired condition.

Signed-off-by: Steve Kriss <stephen.kriss@gmail.com>
Virtually every invocation of these methods ignores
the first return arg, so drop it from the methods.

Signed-off-by: Steve Kriss <stephen.kriss@gmail.com>
Signed-off-by: Steve Kriss <stephen.kriss@gmail.com>
Copy link
Member

@sunjayBhatia sunjayBhatia left a comment

Choose a reason for hiding this comment

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

nice!

@skriss skriss merged commit 136f5a1 into projectcontour:main Apr 30, 2024
26 checks passed
@skriss skriss deleted the pr-e2e-verify-status branch April 30, 2024 19:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-note/none-required Marks a PR as not requiring a release note. Should only be used for very small changes.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants