search

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 changes round 1 #6405

Open elee1766 opened 1 week ago

elee1766 commented 1 week ago

Change caddytest to allow running one instance of caddy per test

caddytest works checking if there is an admin api mounted, and if it is mounted, calls /load on the api, and tries to run tests. If it is not mounted, it tries to start caddy.

this introduced the following race conditions when running go test ./...

  1. multiple servers started at the same-ish time, meaning that they will race to not only modify the global variable os.Args, but also both execute the not-protected rootCmd at the same time, and one will eventually fail, as only one server may bind 2999.
  2. a config to be loaded before another test finishes, causing a test to fail, with unpredictable behavior. even worse, a config loaded in the middle of another test.

In order to remedy this, some somewhat large changes need to be made.

  1. instead of populating a rootCmd, a factory for creating a rootCmd must instead be initialized. This allows each invokation of Main to have its own cmd. This should hopefully allow multiple caddy instances to run in parallel in the same process
  2. instead of reusing the same caddy server with a process for sequential tests, we will create a new instance of caddy every time. since go tests may be run in any order, this provides more reproducibility in tests.
  3. in order to support features 1 and 2, we add custom {$PLACEHOLDERS} which refer to test-local variables. existing tests are modified to work with such placeholders
elee1766 commented 1 week ago
  1. see cmd/commandfactory for the command factory. Basically you pass in pike-style options and it recomputes the cmd when you call Build().
  2. the old Tester struct has been renamed to Harness, and the new tester struct does not specifically work with a *testing.T
elee1766 commented 6 days ago 
* The blast radius of breakage is unfortunate. There are at least 5 users of the `caddytest` package, which this break all of them. I believe it's possible to avoid the breaking changes, at least by swapping the struct's names and re-adjusting their responsibilities.

I think that the PR should irrevocably break the caddytest package, change the API to something that is better, and force the 5-10 downstream users of caddytest to rewrite. If these usages of caddytest are important, then I will personally rewrite or help to rewrite such tests with the new api in preparation for the merge of this PR.

Alternatively, I can construct a compatibility package (caddytestcompat) that can be marked deprecated that these downstream consumers can consciously chose to not migrate and use the old api.

I am still unhappy with both the Tester and TestHarness api, and the changes i have made so far are simply a proof of concept to make sure that the new caddy instance once a file would work. I think more breaking changes need will need to happen. I refuse to succumb to sunk cost in this case, and this amount of usages i do not think is wide spread at all.

mholt commented 3 days ago

Yeah, I am not too worried about breakage at this point, since these are just test APIs, and we've never really formalized them or intended them to be used much. I'm hoping these new changes will be more permanent, more flexible, more useful to plugin authors, etc, that we can proudly document and demonstrate.

Thanks for working on this! I have a big backlog but will hope to join the reviews at some point soon. Don't let me hold things back though.