Skip to content

Conversation

@maxcao13
Copy link
Contributor

@maxcao13 maxcao13 commented May 5, 2025

Refactors the cloudevent_source_test.go test to rely on events instead of sleeping in order to progress and also improves the cleanup between and after tests. We've noticed flakes with this test where the magic sleep timings don't play well on certain machines and platforms.

This commit also adds several more helper test functions that:

  • Allow you to wait for Kubernetes events after calling a trigger function
  • Allows you to execute commands on pods with TTY turned off (this change is due to several weird test flakes
    we've seen where the output of an exec command returned to the user is unexpected)

Checklist

  • [-] When introducing a new scaler, I agree with the scaling governance policy
  • I have verified that my change is according to the deprecations & breaking changes policy
  • [-] Tests have been added
  • [-] Changelog has been updated and is aligned with our changelog requirements
  • [-] A PR is opened to update our Helm chart (repo) (if applicable, ie. when deployment manifests are modified)
  • [-] A PR is opened to update the documentation on (repo) (if applicable)
  • Commits are signed with Developer Certificate of Origin (DCO - learn more)

@wozniakjan
Copy link
Member

wozniakjan commented May 6, 2025

/run-e2e internal
Update: You can check the progress here

Copy link
Member

@wozniakjan wozniakjan left a comment

Choose a reason for hiding this comment

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

great effort! thank you so much for fixing these time.Sleeps with more deterministic behavior. I think it would benefit greatly from using gomega (no need to add ginkgo to the mix, I think that won't be necessary)

@maxcao13
Copy link
Contributor Author

maxcao13 commented May 6, 2025

Thanks Jan! I've just force pushed some changes, lmk what you think when you have time. :-)

@maxcao13 maxcao13 force-pushed the main branch 3 times, most recently from fe53f0a to ca1fdfb Compare May 6, 2025 23:06
Copy link
Member

@JorTurFer JorTurFer left a comment

Choose a reason for hiding this comment

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

This looks nice! just small nit inline

@maxcao13 maxcao13 force-pushed the main branch 2 times, most recently from 2c9f27c to 881e075 Compare June 9, 2025 16:13
Refactors the cloudevent_source test to rely on events instead of sleeping in order to progress and also improves
the cleanup between and after tests. We've noticed flakes with this test where the magic sleep timings don't
play well on certain machines and platforms.

This commit also adds several more helper test functions that:
* Allow you to wait for Kubernetes events after calling a trigger function
* Allows you to consistently validate a condition up to a duration
* Allows you to execute commands on pods with TTY turned off (this change is due to several weird test flakes
we've seen where the output of an exec command returned to the user is unexpected)

Signed-off-by: Max Cao <macao@redhat.com>
@maxcao13
Copy link
Contributor Author

maxcao13 commented Jun 9, 2025

Thanks! Had to rebase to resolve the merge conflict.

@wozniakjan wozniakjan requested a review from Copilot June 9, 2025 16:17
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR refactors the cloud event source test to improve its reliability by replacing fixed sleep timings with event watchers and by enhancing the test cleanup process. Key changes include:

  • Replacing magic sleep durations with event-based triggers and consistency checks.
  • Adding alternate execution functions for commands without TTY and updating logging messages.
  • Improving cleanup by using t.Cleanup in the main test function.

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
tests/internals/cloudevent_source/cloudevent_source_test.go Refactored several test functions to use event watchers, deferred deletion, and improved logging to reduce flakiness
tests/helper/helper.go Introduced ExecCommandOnSpecificPodWithoutTTY and implemented the KedaConsistently function to support event watching and polling
Comments suppressed due to low confidence (1)

tests/internals/cloudevent_source/cloudevent_source_test.go:623

  • The log message in testErrEventSourceCreation mentions 'include filter' even though the function is focused on testing creation errors. Consider updating the log message to more accurately reflect its purpose (e.g., mention 'creation error') for better clarity.
t.Logf("--- test emitting eventsource about scaledobject err with include filter --- [isClusterScope: %t]", isClusterScope)

@rickbrouwer
Copy link
Member

rickbrouwer commented Jun 15, 2025

/run-e2e internal
Update: You can check the progress here

Copy link
Member

@wozniakjan wozniakjan left a comment

Choose a reason for hiding this comment

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

lgtm, thank you!

I could see some minor code structure changes in the future to make the code a bit cleaner, but this fixes a real flaky test so I shouldn't dwell on nitpicks :)

@wozniakjan wozniakjan merged commit 30a37e6 into kedacore:main Jun 20, 2025
20 of 22 checks passed
dpochopsky pushed a commit to dpochopsky/keda that referenced this pull request Sep 12, 2025
Refactors the cloudevent_source test to rely on events instead of sleeping in order to progress and also improves
the cleanup between and after tests. We've noticed flakes with this test where the magic sleep timings don't
play well on certain machines and platforms.

This commit also adds several more helper test functions that:
* Allow you to wait for Kubernetes events after calling a trigger function
* Allows you to consistently validate a condition up to a duration
* Allows you to execute commands on pods with TTY turned off (this change is due to several weird test flakes
we've seen where the output of an exec command returned to the user is unexpected)

Signed-off-by: Max Cao <macao@redhat.com>
Signed-off-by: David Pochopsky <david.pochopsky@united.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants