caddyserver / caddy

Fast and extensible multi-platform HTTP/1-2-3 web server with automatic HTTPS
https://caddyserver.com
Apache License 2.0
55.45k stars 3.91k forks source link

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

Open mohammed90 opened 1 week ago

mohammed90 commented 1 week ago

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.

elee1766 commented 1 week ago

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