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

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from
Draft

Conversation

elee1766
Copy link
Contributor

@elee1766 elee1766 commented Jun 19, 2024

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
Copy link
Contributor Author

  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

logging.go Outdated
Comment on lines 703 to 708
f := flag.Lookup("test.v")
if (f != nil && f.Value.String() != "true") || strings.Contains(os.Args[0], ".test") {
cl.writerOpener = &DiscardWriter{}
} else {
cl.writerOpener = StderrWriter{}
}
Copy link
Member

Choose a reason for hiding this comment

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

I think we do want stderr logs during tests tho, cause it can help trace why a failure happened. Pretty sure go test skips outputting them if there's no failure (unless -v is set)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sounds fine to me. should be resolved.

i will say that i do find it somewhat odd that caddy logs in tests by default. i generally expect go test to run silently.

Copy link
Member

@mohammed90 mohammed90 left a comment

Choose a reason for hiding this comment

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

Thanks for taking a shot at this! It's quite large and a great effort, so I'll try to do it in passes.

I have 2 concerns:

  • 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 fail to see the necessity for the command refactoring. I think the flag check and avoidance of os.Exit can still be done without the refactor. What am I missing?

Comment on lines -781 to +785
os.Exit(exitCode)
// check if we are in test environment, and dont call exit if we are
if flag.Lookup("test.v") == nil && !strings.Contains(os.Args[0], ".test") {
os.Exit(exitCode)
}
Copy link
Member

Choose a reason for hiding this comment

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

My understanding is that the entire rebuild of the command is to avoid this call to os.Exit, evident by the comment inside MainForTesting. Couldn't you do this same check with the previous code as-is? I fail to see the need for the command refactor.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

no.

the reason that we build a new command is to pass args to the main command.

each rootCmd can only be invoked sequentially, because the args passed to the command populate a struct within rootCmd. you can't run two different rootCmd's in parallel.

so rootCmd.SetArgs(args) is the line that requires creating a new command.

"testing"

"github.com/aryann/difflib"
"github.com/stretchr/testify/require"
Copy link
Member

Choose a reason for hiding this comment

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

I just noticed testify is in Caddy's go.mod. The only other usage of it is for the function assert.JSONEq, which isn't complex (merely marshals into interface and deep-compares). We can probably get rid of it if the rest of the team agrees.

Copy link
Contributor Author

@elee1766 elee1766 Jun 24, 2024

Choose a reason for hiding this comment

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

yeah i used it because it was already in the go.mod.

fine with removing it if you guys dont like it. (i'm rather used to this library and use it everywhere, so I preferred to use it when i saw it in the go.mod)

Copy link
Member

Choose a reason for hiding this comment

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

I'm in favor of slimming down our dependencies, especially if it means a change in one other place.

@@ -358,9 +351,9 @@ func TestH2ToH1ChunkedResponse(t *testing.T) {
}
}

func testH2ToH1ChunkedResponseServeH1(t *testing.T) *http.Server {
func testH2ToH1ChunkedResponseServeH1(harness *caddytest.TestHarness, t *testing.T) *http.Server {
Copy link
Member

Choose a reason for hiding this comment

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

Need this be a pointer?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, in case we add something that can't be passed by value, or modify testharness to be a interface.

there is no reason to pass the bare struct

@elee1766
Copy link
Contributor Author

* 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
Copy link
Member

mholt commented Jun 27, 2024

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.

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.

4 participants