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 fix: allow test scenarios run more than one time #10395

Merged
merged 10 commits into from
Nov 26, 2024

Conversation

yuval-k
Copy link
Member

@yuval-k yuval-k commented Nov 22, 2024

Setting the logger in controller runtime only works for the first time, and is no-op the second time. this means that the second time the test runs it will run to a testing.T that finished running.

The fix is to changing the underlying log writer, so we can swap up the testing.T to the current one

Setting the logger in controller runtime only works for the first time, and is no-op the second time. this means that the second time the test runs it will run to a testing.T that finished running.

The fix is to changing the underlying log writer, so we can swap up the testing.T to the current one
func NewTestLogger(t *testing.T) *zap.Logger {
writer := &testingWriter{t: t}
func NewTestLogger() *zap.Logger {
//writer := &testingWriter{t: t}

Choose a reason for hiding this comment

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

nit: can we delete?

also the comment on the constructor is now out of date, since it references testing.T

@yuval-k
Copy link
Member Author

yuval-k commented Nov 22, 2024

/skip-changelog

@yuval-k
Copy link
Member Author

yuval-k commented Nov 22, 2024

/skipCI-kube-tests:true

Copy link

github-actions bot commented Nov 25, 2024

Visit the preview URL for this PR (updated for commit a2f62ce):

https://gloo-edge--pr10395-yuval-k-setup-test-s-8ce4wug3.web.app

(expires Tue, 03 Dec 2024 16:59:40 GMT)

🔥 via Firebase Hosting GitHub Action 🌎

Sign: 77c2b86e287749579b7ff9cadb81e099042ef677

@yuval-k
Copy link
Member Author

yuval-k commented Nov 25, 2024

note: i still see the 0 listener flake; i can't repro locally... I added some more logs in this PR, so next it happens in CI we will have more to go on

@yuval-k
Copy link
Member Author

yuval-k commented Nov 25, 2024

/skip-changelog

@yuval-k yuval-k enabled auto-merge (squash) November 25, 2024 19:00
@yuval-k
Copy link
Member Author

yuval-k commented Nov 25, 2024

/skip-changelog

@yuval-k
Copy link
Member Author

yuval-k commented Nov 26, 2024

/skip-changelog

@yuval-k
Copy link
Member Author

yuval-k commented Nov 26, 2024

/skip-changelog

@yuval-k
Copy link
Member Author

yuval-k commented Nov 26, 2024

/skip-changelog

@yuval-k
Copy link
Member Author

yuval-k commented Nov 26, 2024

/skip-changelog

@yuval-k yuval-k merged commit cdd3ac7 into main Nov 26, 2024
19 checks passed
@yuval-k yuval-k deleted the yuval-k/setup-test-support-count branch November 26, 2024 18:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants