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.

Sign up for GitHub

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

Jump to bottom

🐛 Client go version metrics endpoint test #1821

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

🐛 Client go version metrics endpoint test #1821

wants to merge 8 commits into from
+361 −270

Conversation

bentito
Copy link
Contributor

@bentito bentito commented Feb 27, 2025

Description

This PR rewrites the metrics endpoint e2e test using client-go. It maintains the recent change to the kubectl exec version by still "force" deleting the Pod (0 time grace period). Seems to pass a local 10 of 10 in a row e2e test.

Added an optional TEST_FILTER env var to the e2e run in the Makefile. Not really related but sure is better when you're iterating on one e2e test change.

Reviewer Checklist

  • API Go Documentation
  • Tests: Unit Tests (and E2E Tests, if appropriate)
  • Comprehensive Commit Messages
  • Links to related GitHub Issue(s)

@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 27, 2025
@bentito bentito requested a review from a team as a code owner February 27, 2025 19:10
@netlify Netlify
Copy link

netlify bot commented Feb 27, 2025 •
edited
Loading

✅ Deploy Preview for olmv1 ready!

Name Link
🔨 Latest commit 15a1c2a
🔍 Latest deploy log https://app.netlify.com/sites/olmv1/deploys/67c624d79fb6260008988345
😎 Deploy Preview https://deploy-preview-1821--olmv1.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@bentito bentito force-pushed the client-go-version-metrics-endpoint-test branch from ecfef0f to dce13b3 Compare February 27, 2025 19:26
@openshift-merge-robot openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 27, 2025
@bentito
Copy link
Contributor Author

bentito commented Feb 27, 2025

Hmm, I pulled test/utils.go because I thought it had no used methods once I moved FindK8sClient to the test file and made it local. There was another seemingly unused, but now I'm realizing possibly externally used function there. Should I just put utils.go back as-was to alleviate the go-apidiff check fail, or...?

@codecov Codecov
Copy link

codecov bot commented Feb 27, 2025 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 68.44%. Comparing base (97b1337) to head (15a1c2a).
Report is 1 commits behind head on main.

Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #1821      +/-   ##
==========================================
- Coverage   68.50%   68.44%   -0.06%     
==========================================
  Files          63       63              
  Lines        5134     5134              
==========================================
- Hits         3517     3514       -3     
- Misses       1388     1390       +2     
- Partials      229      230       +1
Flag Coverage Δ
e2e 51.57% <ø> (-0.16%) ⬇️
unit 56.03% <ø> (ø)

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.

camilamacedo86
camilamacedo86 reviewed Feb 28, 2025
View reviewed changes
test/e2e/metrics_test.go
require.Contains(c.t, string(output), "200 OK", "Metrics endpoint did not return 200 OK")
// Combine stdout + stderr
combined := stdout.String() + stderr.String()
require.Contains(c.t, combined, "200 OK", "Metrics endpoint did not return 200 OK")
Copy link
Contributor

@camilamacedo86 camilamacedo86 Feb 28, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Is it not much more complex than before?
I think before it was very clear what it does, we could easily copy and paste to validate the things.
But anyone, maybe it is a matter of preference. So, OK with ANY

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah Idk, it was certainly harder to implement, kubectl exec was doing several things for us. I think this PR mostly just answers @joelanford concern that that help from kubectl also opened more avenues for things to go subtly wrong. And @azych desire that this test should use go-client like the other e2e.

Copy link
Member

Choose a reason for hiding this comment

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

I could see wrapping this in a more general purpose execInPod kind of utility function that could be re-used. But mostly a nit since there's not another place that we need this at the moment.

azych
azych reviewed Feb 28, 2025
View reviewed changes
azych
azych approved these changes Feb 28, 2025
View reviewed changes
Copy link
Contributor

@azych azych left a comment

Choose a reason for hiding this comment

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

LGTM, just added a few nits

joelanford
joelanford reviewed Mar 3, 2025
View reviewed changes
@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 3, 2025
bentito added 8 commits March 3, 2025 16:51
@bentito
Adds TEST_FILTER env var to e2e run
b34f3bf 
Signed-off-by: Brett Tofel <[email protected]>
@bentito
Rewrite metrics endpoint test to use client-go
363bf1d 
test/utils.go deleted because no used funcs

Signed-off-by: Brett Tofel <[email protected]>
@bentito
Rm pointer helpers, use k8s ones.
ab09759 
Signed-off-by: Brett Tofel <[email protected]>
@bentito
Rename main test client to globalClient (de-shadow "c")
c6c9b21 
Signed-off-by: Brett Tofel <[email protected]>
@bentito
Use apierrors, move deferred cleanup, less nesting
37a5453 
Signed-off-by: Brett Tofel <[email protected]>
@bentito
Rename main test cfg->globalConfig de-shadow "cfg"
1631058 
Signed-off-by: Brett Tofel <[email protected]>
@bentito
Allow test-unit filter on package and test
cf22480 
Have to have package filter too, to not run all the unit tests due to UNIT_TEST_DIRS

Signed-off-by: Brett Tofel <[email protected]>
@bentito
Drop UNIT_TEST_DIRS for TEST_PKGS + TEST_FILTER
15a1c2a 
Signed-off-by: Brett Tofel <[email protected]>
@bentito bentito force-pushed the client-go-version-metrics-endpoint-test branch from 8f5b7eb to 15a1c2a Compare March 3, 2025 21:53
@openshift-merge-robot openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 3, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@joelanford joelanford joelanford left review comments

@perdasilva perdasilva perdasilva left review comments

@LalatenduMohanty LalatenduMohanty LalatenduMohanty left review comments

@camilamacedo86 camilamacedo86 camilamacedo86 left review comments

@azych azych azych approved these changes

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@bentito @joelanford @perdasilva @LalatenduMohanty @azych @camilamacedo86 @openshift-merge-robot