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

v2: Writing unit tests for Caddyfile parsing logic #3402

Closed
kujenga opened this issue May 12, 2020 · 8 comments · Fixed by #3439
Closed

v2: Writing unit tests for Caddyfile parsing logic #3402

kujenga opened this issue May 12, 2020 · 8 comments · Fixed by #3439
Labels
discussion 💬 The right solution needs to be found

Comments

@kujenga
Copy link
Contributor

kujenga commented May 12, 2020

I'm looking at porting a plugin from v1 to v2 and have gotten stuck migrating over tests. I previously had tests that utilized the caddy.NewTestController method on Caddy v1, in order to pass that test controller to the same setup function that I used in the initialization of my plugin.

	caddy.RegisterPlugin("my_plugin", caddy.Plugin{
		ServerType: "http",
		Action:     setup,
	})

At a high level, my goals are to port over the following types of tests:

  • unit tests that assert that a given set of Caddyfile directives are parsed in the expected manner. The tests currently take in values like the following, run them through caddy.NewTestController, and setup the plugin from there. They then verify that the output configuration looks as expected.
my_plugin test:1234 {
    key value
    other
}
  • Tests that verify that the middleware is present in the Caddy server, and inspect values on it to make sure that it is configured correctly. An example of this:
ctrlr := caddy.NewTestController(...)
setup(ctrlr)

cfg := httpserver.GetConfig(ctrlr)
v = cfg.Middleware()[0](nil).(*MyPlugin)

// ...various assertions

I started looking at the httpcaddyfile.ServerType.Setup [1] function as a possible replacement but it seemed unclear if that was a productive route to go. Maybe there is a better way to test plugins that requires re-thinking tests more holistically as part of porting them off of v1?

[1] https://pkg.go.dev/github.com/caddyserver/caddy/[email protected]/caddyconfig/httpcaddyfile?tab=doc#ServerType.Setup

@francislavoie
Copy link
Member

francislavoie commented May 12, 2020

There's a handful of helpers in the caddytest package that you could use, such as AssertAdapt, but unfortunately that approach isn't very isolated. See https://github.com/caddyserver/caddy/blob/master/caddytest/caddytest.go.

BTW you linked docs from a pretty old version of Caddy, that's from beta 11, from December. Things have changed quite a bit since then!

@mholt
Copy link
Member

mholt commented May 12, 2020

@kujenga Thanks for working on a Caddy plugin!

In v2, you'll implement the caddyfile.Unmarshaler interface: https://pkg.go.dev/github.com/caddyserver/caddy/[email protected]/caddyconfig/caddyfile?tab=doc#Unmarshaler - this gives you a Dispenser; in tests you can call NewDispenser: https://pkg.go.dev/github.com/caddyserver/caddy/[email protected]/caddyconfig/caddyfile?tab=doc#NewDispenser

So I guess we just need an easy way to get []Token from a Caddyfile string?

BTW you linked docs from a pretty old version of Caddy, that's from beta 11, from December. Things have changed quite a bit since then!

That's probably my fault; when the Extending Caddy docs page was written, pkg.go.dev wouldn't link to v2 docs because it only had prerelease tags, so I had to hard-code a version. Now that v2.0.0 is released (a non-prerelease tag), I can update the links. (Done locally, will push.)

@francislavoie
Copy link
Member

I'm interested in trying to improve the Caddyfile testing helpers, I might try to tackle some of these things soon (but don't let me block you, go ahead and try things if you get to it first!)

@kujenga
Copy link
Contributor Author

kujenga commented May 12, 2020

Thanks for the quick responses! I fixed the docs link in the original ticket, it looks like the specific function I'd linked to had no changes, and I was looking at the latest version of the code locally.

Based on the pointer to NewDispenser I do see a NewTestDispenser in this test file that seems to do at least part of what is wanted, but it's not really exposed in a way that seems accessible to third party packages:

// NewTestDispenser parses input into tokens and creates a new
// Disenser for test purposes only; any errors are fatal.
func NewTestDispenser(input string) *Dispenser {
tokens, err := allTokens("Testfile", []byte(input))
if err != nil && err != io.EOF {
log.Fatalf("getting all tokens from input: %v", err)
}
return NewDispenser(tokens)
}

I'll be looking into AssertAdapt and the like as well!

@mholt
Copy link
Member

mholt commented May 13, 2020

@kujenga Ah, indeed - we just need to move that into a non-test file.

@kujenga
Copy link
Contributor Author

kujenga commented May 16, 2020

Below for reference is the hack I ended up with just to get tests passing again, the idea is to go from Caddyfile -> actual instantiated module. With the instantiated module initialized by Caddy internals, I then perform unit tests on that module, asserting it's properly initialized, does the right things to requests, etc.

It seems like the missing piece here for me is: Is there a better way to have Caddy instantiate a module from a server configuration, and then give a test access to an instance of that module?

Part of the answer here might be that this sort of test is just less worthwhile than it was before in v1, since there's significantly less that can go wrong when the module initialization seems to be effectively just JSON deserialization followed by calling the Provision method.

cfgAdapter := caddyconfig.GetAdapter("caddyfile")
require.NotNil(t, cfgAdapter)
result, warnings, err := cfgAdapter.Adapt([]byte(fullCaddyfile), nil)
assert.Empty(t, warnings, "warnings adapting caddyfile")
require.NoError(t, err, tc.input, "adopting caddyfile")

// Load the Gizmo module from JSON output
var allObj map[string]interface{}
require.NoError(t, json.Unmarshal(result, &allObj))
type m map[string]interface{}
type a []interface{}
modObj := allObj["apps"].(map[string]interface{})["http"]
modObj = modObj.(map[string]interface{})["servers"]
modObj = modObj.(map[string]interface{})["srv0"]
modObj = modObj.(map[string]interface{})["routes"]
modObj = modObj.([]interface{})[0]
modObj = modObj.(map[string]interface{})["handle"]
modObj = modObj.([]interface{})[0]
modObj = modObj.(map[string]interface{})["routes"]
modObj = modObj.([]interface{})[0]
modObj = modObj.(map[string]interface{})["handle"]
modObj = modObj.([]interface{})[0]
delete(modObj.(map[string]interface{}), "handler")
modRaw, err := json.Marshal(modObj)
require.NoError(t, err)

// Load module from JSON output
cctx, cancel := caddy.NewContext(caddy.Context{Context: ctx})
defer cancel()
mod, err := cctx.LoadModuleByID("http.handlers.gizmo", (json.RawMessage)(modRaw))
require.NoError(t, err, "loading module by ID")
gizmo := mod.(*Gizmo)

// Test the gizmo...

@mholt
Copy link
Member

mholt commented May 21, 2020

@kujenga Interesting, thanks for sharing that. Does moving NewTestDispenser out of a _test file help simplify that at all?

@mholt
Copy link
Member

mholt commented May 27, 2020

We made NewTestDispenser() a non-test function so you can test your Caddyfile unmarshaling code easier.

@kujenga

Part of the answer here might be that this sort of test is just less worthwhile than it was before in v1, since there's significantly less that can go wrong when the module initialization seems to be effectively just JSON deserialization followed by calling the Provision method.

Yeah, that's basically what it comes down to I think. You definitely need to be able to test your Caddyfile unmarshaler (which we just made possible) -- but after that, it's on Caddy to load your module successfully. Most of your snippet above is testing logic in Caddy that Caddy is/should already be testing.

You should probably test your own Provision and Validate methods (if you implement them -- they're optional) but I don't think you need much scaffolding for that -- just a caddy.Context, which is easy to make, I think.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discussion 💬 The right solution needs to be found
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants