✨ (go/v4) Add Metrics Validation and Helper Functions to E2E Tests #4131

mogsie · 2024-09-04T18:04:04Z

Provide further improvements for e2e tests test to help users be aware of how to tests using the metrics endpoint and validate if the metrics are properly expose. Furthermore, shows how to use it to validate the reconciliation.

Closes: #4124

Partially closes: #4120

linux-foundation-easycla · 2024-09-04T18:04:08Z

The committers listed above are authorized under a signed CLA.

✅ login: camilamacedo86 / name: Camila Macedo (d279035)

k8s-ci-robot · 2024-09-04T18:04:14Z

Hi @mogsie. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

camilamacedo86

Hi @mogsie

That is great !!! Well done 🎉
Just a couple of nits:

Can you please squash all commits? We need to have 1 commit per PR before merge
2 see that to update all samples and docs we need to run make install and then, make generate. In this way, all will be re-scaffolded
For all eventually checks, could we run the check after each 10 seconds? So that we avoid see so many lines in the log trying to retrieve the staff?

So, can you please do both for we get this one merged (assuming that will pass in the tests CI)

camilamacedo86 · 2024-09-05T05:32:29Z

docs/book/src/cronjob-tutorial/testdata/project/test/e2e/e2e_test.go

 			}
 			// Repeatedly check if the controller-manager pod is running until it succeeds or times out.
-			EventuallyWithOffset(1, verifyControllerUp, time.Minute, time.Second).Should(Succeed())
+			Eventually(verifyControllerUp, time.Minute, time.Second).Should(Succeed())


Suggested change

Eventually(verifyControllerUp, time.Minute, time.Second).Should(Succeed())

Eventually(verifyControllerUp, time.Minute, 10* time.Second).Should(Succeed())

Could we test each 10 seconds?

Oh, my bad, I botched this syntax. It should have been

Suggested change

Eventually(verifyControllerUp, time.Minute, time.Second).Should(Succeed())

Eventually(verifyControllerUp).WithTimeout(time.Minute).WithInterval(time.Second).Should(Succeed())

The default interval is 100ms, I think, so that's why the log was filled with these.

I think I can configure the timeout and intervals at the top of the test instead. No need to litter the code with this.

camilamacedo86 · 2024-09-05T05:32:58Z

docs/book/src/cronjob-tutorial/testdata/project/test/e2e/e2e_test.go

+				g.Expect(string(logs)).To(ContainSubstring("controller-runtime.metrics\tServing metrics server"),
+					"Metrics server not yet started")
+			}
+			Eventually(verifyMetricsServerStarted).WithTimeout(2 * time.Minute).Should(Succeed())


Suggested change

Eventually(verifyMetricsServerStarted).WithTimeout(2 * time.Minute).Should(Succeed())

Eventually(verifyMetricsServerStarted).WithTimeout(2 * time.Minute, 10 * time.Seconds).Should(Succeed())

Suggested change

Eventually(verifyMetricsServerStarted).WithTimeout(2 * time.Minute).Should(Succeed())

Eventually(verifyControllerUp, time.Minute, 10* time.Second).Should(Succeed())

Could we test each after 10 seconds?

I added a default interval of 1 second and timeout of 2 minutes, so I could remove most of the WithTimeout calls. hopefully the logs won't contain more than a few lines of repeated log entries when Eventuallying.

camilamacedo86 · 2024-09-05T05:33:16Z

docs/book/src/cronjob-tutorial/testdata/project/test/e2e/e2e_test.go

+					"-n", namespace)
+				g.Expect(utils.Run(cmd)).To(BeEquivalentTo("Succeeded"), "curl pod in wrong status")
+			}
+			Eventually(verifyCurlUp).WithTimeout(5 * time.Minute).Should(Succeed())


same here:

Could we test each after 10 seconds?

mogsie · 2024-09-05T06:24:35Z

I only really concentrated on the first test; I wanted to know if I was overreaching or if this was going in the right direction. I will extend the change to the whole file later. 👍

mogsie · 2024-09-05T06:26:12Z

I based my PR off your work; I could rebase this off #4124, if you prefer (to keep things separate).

Edit Oh, I see you prefer this PR to 4124. I'll just squash it all then, when I continue.

camilamacedo86 · 2024-09-05T13:49:13Z

I based my PR off your work; I could rebase this off #4124, if you prefer (to keep things separate).

Yes, just squash all.

Provide further improvements for e2e tests test to help users be aware of how to tests using the metrics endpoint and validate if the metrics are properly expose.

camilamacedo86

/lgtm
/ok-to-test
/approved

Great work !!!

k8s-ci-robot · 2024-09-06T06:50:43Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: camilamacedo86, mogsie

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

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [camilamacedo86]

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

k8s-ci-robot requested review from camilamacedo86 and everettraven September 4, 2024 18:04

k8s-ci-robot added cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Sep 4, 2024

mogsie force-pushed the test-e2e-impro branch from e9227fb to 1151a1a Compare September 4, 2024 20:38

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Sep 4, 2024

mogsie force-pushed the test-e2e-impro branch 2 times, most recently from afc7d77 to 6b67366 Compare September 4, 2024 21:53

camilamacedo86 reviewed Sep 5, 2024

View reviewed changes

camilamacedo86 self-requested a review September 5, 2024 15:55

✨ add metrics check and further helpers for the e2e tests

d279035

Provide further improvements for e2e tests test to help users be aware of how to tests using the metrics endpoint and validate if the metrics are properly expose.

mogsie force-pushed the test-e2e-impro branch from 6b67366 to d279035 Compare September 5, 2024 19:19

camilamacedo86 approved these changes Sep 6, 2024

View reviewed changes

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Sep 6, 2024

k8s-ci-robot assigned camilamacedo86 Sep 6, 2024

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 6, 2024

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 6, 2024

camilamacedo86 mentioned this pull request Sep 6, 2024

✨ (go/v4) Add scaffold for e2e webhook checks #4121

Merged

k8s-ci-robot merged commit 2d34685 into kubernetes-sigs:master Sep 6, 2024
18 checks passed

camilamacedo86 mentioned this pull request Sep 6, 2024

E2e tests scaffolded by default #4120

Closed

mogsie deleted the test-e2e-impro branch September 7, 2024 11:28

camilamacedo86 mentioned this pull request Oct 10, 2024

Proposed Changes for 4.3.0 Release #4214

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

✨ (go/v4) Add Metrics Validation and Helper Functions to E2E Tests #4131

✨ (go/v4) Add Metrics Validation and Helper Functions to E2E Tests #4131

mogsie commented Sep 4, 2024 •

edited by camilamacedo86

Loading

linux-foundation-easycla bot commented Sep 4, 2024 •

edited

Loading

k8s-ci-robot commented Sep 4, 2024

camilamacedo86 left a comment •

edited

Loading

camilamacedo86 Sep 5, 2024 •

edited

Loading

mogsie Sep 5, 2024

camilamacedo86 Sep 5, 2024 •

edited

Loading

mogsie Sep 5, 2024

camilamacedo86 Sep 5, 2024 •

edited

Loading

mogsie commented Sep 5, 2024

mogsie commented Sep 5, 2024 •

edited

Loading

camilamacedo86 commented Sep 5, 2024

camilamacedo86 left a comment

k8s-ci-robot commented Sep 6, 2024

	Eventually(verifyControllerUp, time.Minute, time.Second).Should(Succeed())
	Eventually(verifyControllerUp, time.Minute, 10* time.Second).Should(Succeed())

	Eventually(verifyControllerUp, time.Minute, time.Second).Should(Succeed())
	Eventually(verifyControllerUp).WithTimeout(time.Minute).WithInterval(time.Second).Should(Succeed())

	Eventually(verifyMetricsServerStarted).WithTimeout(2 * time.Minute).Should(Succeed())
	Eventually(verifyMetricsServerStarted).WithTimeout(2 * time.Minute, 10 * time.Seconds).Should(Succeed())

✨ (go/v4) Add Metrics Validation and Helper Functions to E2E Tests #4131

✨ (go/v4) Add Metrics Validation and Helper Functions to E2E Tests #4131

Conversation

mogsie commented Sep 4, 2024 • edited by camilamacedo86 Loading

linux-foundation-easycla bot commented Sep 4, 2024 • edited Loading

k8s-ci-robot commented Sep 4, 2024

camilamacedo86 left a comment • edited Loading

Choose a reason for hiding this comment

camilamacedo86 Sep 5, 2024 • edited Loading

Choose a reason for hiding this comment

mogsie Sep 5, 2024

Choose a reason for hiding this comment

camilamacedo86 Sep 5, 2024 • edited Loading

Choose a reason for hiding this comment

mogsie Sep 5, 2024

Choose a reason for hiding this comment

camilamacedo86 Sep 5, 2024 • edited Loading

Choose a reason for hiding this comment

mogsie commented Sep 5, 2024

mogsie commented Sep 5, 2024 • edited Loading

camilamacedo86 commented Sep 5, 2024

camilamacedo86 left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Sep 6, 2024

mogsie commented Sep 4, 2024 •

edited by camilamacedo86

Loading

linux-foundation-easycla bot commented Sep 4, 2024 •

edited

Loading

camilamacedo86 left a comment •

edited

Loading

camilamacedo86 Sep 5, 2024 •

edited

Loading

camilamacedo86 Sep 5, 2024 •

edited

Loading

camilamacedo86 Sep 5, 2024 •

edited

Loading

mogsie commented Sep 5, 2024 •

edited

Loading