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

caddytest: delete existing config at end of test run #6414

Closed
wants to merge 1 commit into from

Conversation

mohammed90
Copy link
Member

While #6405 is in progress, I realized one of the issues is that tests configs step on each other's toes. Although the tests are not parallel, there's some async process causing them to step on each other. I wonder if this step merely band-aids the bigger issue or it actually resolves the underlying issue.

CC/ @elee1766, since you're working on the other PR and engaging in discussion in Slack

I also noticed we're not using the HTTP Client that's in Tester everywhere. It's not used in initServer nor in ensureConfigRunning and perhaps in a few other places. My memory doesn't serve me if that's deliberate or just a mistake.

@mohammed90 mohammed90 added the CI/CD 🔩 Automated tests, releases label Jun 20, 2024
@mohammed90 mohammed90 added this to the v2.8.5 milestone Jun 20, 2024
@mohammed90 mohammed90 requested a review from francislavoie June 20, 2024 08:53
@mohammed90 mohammed90 force-pushed the delete-config-test-cleanup branch from 55bd9da to bdb5225 Compare June 20, 2024 09:02
@elee1766
Copy link
Contributor

elee1766 commented Jun 20, 2024

answer question:

I don't think not unloading the config is the problem itself. caddy switching configs should be able to honor the test just as well as not switching configs. we wait for config load to finish, worse case you could get side effects from an old server?

the http client in tester - I don't really know what it's there for. I left it in but i also found no uses. I was going to go through the blame later and figure out who made it originally

some notes:

tests within a package are run in sequence, but tests in different packages may be running in parallel. this causes the existing caddytest framework to flake because you have one server trying to run two tests, loading multiple configs at once, so of course one fails the test (since they r bound to the same port) this is a major underlying issue.

so many races are happening because of that. you can test this because you will notice running on an individual package almost never flakes, but do go test ./... in the caddytest folder and you start getting flaky results

@mohammed90
Copy link
Member Author

Closing it as it's not needed

@mohammed90 mohammed90 closed this Aug 28, 2024
@mohammed90 mohammed90 deleted the delete-config-test-cleanup branch August 28, 2024 06:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI/CD 🔩 Automated tests, releases
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants