chore: refactor tests to passthrough custom frontends #56
Conversation
cpuguy83
changed the title
|
@pmengelbert @adamperlin Could use a review on this one to get us into better shape testing wise. |
|
@cpuguy83 Still catching up from the break, but this is on my list. |
This changes the integration test suite such that custom frontends may
now be injected into the test environment.
The base frontend ref is no longer given to the test function.
If a test wants to reference the base frontend image it can use
`withFrontend("someName", buildLocalFrontend)`.
Refactors the test handler test to use a custom frontend so that we can
actually verify that forwarding is happening as expected.
This does a bit more testing with a bit less code. It removes some redundant tests cases as well as validate some of the target listing behavior. Signed-off-by: Brian Goff <[email protected]>
This makes it so there's less magic routing a requested frontend image to the right place by making the caller generate the reference ahead of time. This provides a function to handle that generation which determines how to generate it based on if named contexts are used or if the frontend needs to go to a registry. Signed-off-by: Brian Goff <[email protected]>
Signed-off-by: Brian Goff <[email protected]>
Signed-off-by: Brian Goff <[email protected]>
The test env config is now in a separate package. Client wrapping is now simplified (and working). This change makes it so buildkit v0.12 is a requirement. For now, when buildkit v0.12 is not available it errors out. Signed-off-by: Brian Goff <[email protected]>
This opens us up to support other means of managing custom frontends than just buildkit v0.12 + inputs. It doesn't add anything new, but makes what we have more open to it. Signed-off-by: Brian Goff <[email protected]>
This just removes some extra boiler plate for error checking things when reading or stating a file in a build result. Signed-off-by: Brian Goff <[email protected]>
Signed-off-by: Brian Goff <[email protected]>
Signed-off-by: Brian Goff <[email protected]>
This simplifies how to execute a test and reduces duplicate code required to load frontends for a test. This should also make it easier to support buildkit < v0.12 for tests, however this support is still not implemented. Signed-off-by: Brian Goff <[email protected]>
Signed-off-by: Brian Goff <[email protected]>
This is needed for integration tests, but should be generally useful. Signed-off-by: Brian Goff <[email protected]>
|
Feedback addressed. |
When using a docker image source and running multiple cmd steps, any state changes that occurred before the final step was not being preserved due to how llb mounts are being used to capture subpaths. This fixes that issue and adds an integration test for it. This test largely, but not completely (yet) replaces `test/fixtures/cmd-src-ref.yml` Signed-off-by: Brian Goff <[email protected]>
Signed-off-by: Brian Goff <[email protected]>
This makes creating a request a bit less versbose by using functional options to apply common things like build targets and injecting the spec. Signed-off-by: Brian Goff <[email protected]>
Signed-off-by: Brian Goff <[email protected]>
|
Anything else here? |
| Build gwclient.BuildFunc | ||
| } | ||
|
|
||
| func (b *BuildxEnv) RunTest(ctx context.Context, t *testing.T, f gwclient.BuildFunc) { |
There was a problem hiding this comment.
can you rename f to testFunc or something like that?
| } | ||
|
|
||
| func (c *clientForceDalecWithInput) Solve(ctx context.Context, req gwclient.SolveRequest) (*gwclient.Result, error) { | ||
| if req.Definition == nil { |
There was a problem hiding this comment.
When would we hit this case?
There was a problem hiding this comment.
For the actual tests where we are using a spec and/or a build context this will be nil.
For building the frontend (like phony) this will be non-nil.
The reason this is here is because we can't inject FrontendInputs when Definition is set, buildkit will error out.
There was a problem hiding this comment.
Got it, ok. Can you add a comment basically saying this?
| withGHCache(t, &so) | ||
|
|
||
| _, err = c.Build(ctx, so, "", func(ctx context.Context, gwc gwclient.Client) (*gwclient.Result, error) { | ||
| gwc = &clientForceDalecWithInput{gwc} |
There was a problem hiding this comment.
Can you add a comment basically explaining this client layering logic a bit more? I.e., by default, we are using a client that will force the use of the target frontend, but if any references are provided, we will create (potentially multiple) layers of wrapper gwClientInputInject clients that inject inputs with their Solve methods, before returning to the Solve in the client they wrap? It's kind of an override method type pattern almost
There was a problem hiding this comment.
These are really documented on the types themselves.
Is there something more you are looking for?
This changes the integration test suite such that custom frontends may
now be injected into the test environment.
Refactors the test handler test to use a custom frontend so that we can
actually verify that forwarding is happening as expected.
Additionally adds some extra tests (from the older tests) and fixed a bug found after migrating those tests along the way.
Closes #52