8000 test: fix docker-gen tests flakyness by buchdag · Pull Request #2625 · nginx-proxy/nginx-proxy · GitHub
[go: up one dir, main page]
Skip to content

test: fix docker-gen tests flakyness #2625

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
Merged
merged 6 commits into from
May 31, 2025
Merged

test: fix docker-gen tests flakyness #2625

merged 6 commits into from
May 31, 2025

Conversation

buchdag
Copy link
Member
@buchdag buchdag commented May 23, 2025 •
edited
Loading

Tests introduced in #2279 where very flaky because they used the docker_container_dns_resolver, this PR should fix this.

I've ran the tests on this branch four times in a row without issue.

@SchoNie could you confirm ?

@buchdag buchdag self-assigned this May 23, 2025
@buchdag buchdag added the type/test PR that add missing tests or correct existing tests label May 23, 2025
@buchdag buchdag mentioned this pull request May 23, 2025
Closed
buchdag
buchdag commented May 24, 2025
View reviewed changes
@SchoNie
Copy link
Contributor
SchoNie commented May 26, 2025

For me tests still failed 1 out of 5 runs.
action 42880809287

@buchdag
Copy link
Member Author
buchdag commented May 26, 2025 •
edited
Loading

@SchoNie the test failing in this run is a different one : test_forwards_to_whoami.

It's been flaky for a while now, I'm not really sure of why but that's the only test of the whole test suite that use jwilder/whoami, so this might be related. no, it's not using jwilder/whoami at all.

@buchdag
test: drop old checks in docker-gen tests
d3a3d32 
docker engine 1.13 has been out for more than 8 years now
@buchdag buchdag force-pushed the test/wait-for-docker-gen branch from d3ca172 to d3a3d32 Compare May 26, 2025 20:59
@SchoNie
Copy link
Contributor
SchoNie commented May 27, 2025 •
edited
Loading

@SchoNie the test failing in this run is a different one : test_forwards_to_whoami.

It's been flaky for a while now, I'm not really sure of why but that's the only test of the whole test suite that use jwilder/whoami, so this might be related. no, it's not using jwilder/whoami at all.

I was already confused about the jwilder/whoami but you edited the comment.
Ran them a few more times and had 1 fail, it showed an error like [emerg] pread() returned only xxx bytes instead of yyy in /etc/nginx/conf.d/default.conf so maybe there is a race condition with docker-gen writing and nginx loading the config (while still writing) ? 😕
Which could explain why it only sometimes happens....
Is it an idea to add a depends_on to control startup order?

I don't see anything wrong with the tests now and your docker_container_dns_resolver change fixed the empty ip.

@buchdag
Copy link
Member Author
buchdag commented May 31, 2025

Ran them a few more times and had 1 fail, it showed an error like [emerg] pread() returned only xxx bytes instead of yyy in /etc/nginx/conf.d/default.conf so maybe there is a race condition with docker-gen writing and nginx loading the config (while still writing) ?

Do you remember if the actual bytes read from the files where zero (ie a truncated file) or positive ? 🤔

Anyway I think I'll merge this PR like this and I'll maybe come back to the remaining flaky test later.

@buchdag buchdag merged commit 219d3b1 into main May 31, 2025
5 checks passed
@buchdag buchdag deleted the test/wait-for-docker-gen branch May 31, 2025 19:17
@SchoNie
Copy link
Contribut 8AC1 or
SchoNie commented Jun 2, 2025 •
edited
Loading

Do you remember if the actual bytes read from the files where zero (ie a truncated file) or positive ? 🤔

They were positive. What I remember it showed something like 850 bytes instead of 1350, sorry I lost the run logs.

Anyway I think I'll merge this PR like this and I'll maybe come back to the remaining flaky test later.

👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@SchoNie SchoNie SchoNie approved these changes

Assignees

@buchdag buchdag

Labels
type/test PR that add missing tests or correct existing tests
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@buchdag @SchoNie
0