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

refactor(kiln test): encapsulate containerized tile testing #420

Merged
merged 13 commits into from
Jul 20, 2023

Conversation

crhntr
Copy link
Member

@crhntr crhntr commented Jul 18, 2023

This change introduces a new private package called "test" (full name "github.com/pivotal-cf/kiln/internal/test"). It exports two identifiers. test.Run and test.Configuration. The former is a function with signature:

func Run(ctx context.Context, w io.Writer, configuration Configuration) error { return nil }

The Configuration structure is defined as:

type Configuration struct {
	SSHSocketAddress string

	AbsoluteTileDirectory string

	RunAll,
	RunMigrations,
	RunManifest,
	RunMetadata bool

	GinkgoFlags string
	Environment []string
}

All the internal docker/ssh-agent configuration is unit tested via package internal tests so that the public interface used in the commands package is easier to test and understand.

Fixes

This fixes an issue where all kiln commands will fail if the ssh-agent is not properly configured. Now it will only fail on kiln test.

This makes the command tests run much faster and makes command tests run on the order of seconds not minutes. By default the integration test between the ./internal/test package and Docker is run; however, for fast feedback a kiln developer can run test with the -short flag. On my machine I can run go test -short ./... from the project root and it finishes in a couple seconds.

@cf-gitbot
Copy link
Member

We have created an issue in Pivotal Tracker to manage this. Unfortunately, the Pivotal Tracker project is private so you may be unable to view the contents of the story.

The labels on this github issue will be updated when the story is started.

@crhntr crhntr requested a review from notrepo05 July 18, 2023 00:02
@crhntr crhntr changed the title refactor kiln test to use a domain package refactor kiln test to use package encapsulating containerized tile test functionality Jul 18, 2023
@crhntr crhntr changed the title refactor kiln test to use package encapsulating containerized tile test functionality refactor kiln test to use a function encapsulating containerized tile test functionality Jul 18, 2023
@crhntr crhntr changed the title refactor kiln test to use a function encapsulating containerized tile test functionality refactor(kiln test): to use a function encapsulating containerized tile test functionality Jul 18, 2023
@crhntr crhntr force-pushed the refactor-kiln-test branch 3 times, most recently from 70486e9 to bb85f96 Compare July 18, 2023 00:17
@crhntr crhntr changed the title refactor(kiln test): to use a function encapsulating containerized tile test functionality refactor(kiln test): encapsulate containerized tile test functionality Jul 18, 2023
@crhntr crhntr changed the title refactor(kiln test): encapsulate containerized tile test functionality refactor(kiln test): encapsulate containerized tile testing Jul 18, 2023
Copy link
Contributor

@notrepo05 notrepo05 left a comment

Choose a reason for hiding this comment

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

I'd suggest a failure case due to the high impact on CI if we fail to return the error properly. Or fix this if there's an issue in implementation. Looks great though!

internal/test/integration_test.go Show resolved Hide resolved
@crhntr crhntr requested a review from notrepo05 July 18, 2023 16:22
@crhntr crhntr force-pushed the refactor-kiln-test branch 6 times, most recently from d27d607 to a3ea6c9 Compare July 18, 2023 18:51
By moving the tile test code to a separate directory, the command code can focus on flag parsing and user experience.
The tile package can be freely factored to improve understanding of the container interactions.

This change also fixes some conncurrency bugs that led to leaked go-routines.

It should have nearly identical behavior from an end user perspective with a few small changes:
- some short flags were removed (multi character short flags are weird, if a single character does not make sense, remove it).

Also the test of a failing tile test was removed. This could easily be added back in.
A unit test covers a similar scenario and the integration test in the test package only tests the success case.

If you run 'go test -short ./...' from the root of the project the test should run in a few seconds (rather than a few minutes).
…in github actios

failed to solve with frontend dockerfile.v0: failed to solve with frontend gateway.v0: no active session for some-id: context deadline exceeded
there was a deprecation warning in the GitHub ui
crhntr and others added 5 commits July 20, 2023 14:42
also fmt

Co-authored-by: Nick Rohn <[email protected]>
this improves traceability
Co-authored-by: Nick Rohn <[email protected]>

not sure if this is helpful but it clairifies where we need this construct
this was a mistake. if a err fn succeeds (does not return an error) the context is cancelled. This might be the error.
@notrepo05 notrepo05 merged commit cb8096e into main Jul 20, 2023
3 checks passed
@notrepo05 notrepo05 deleted the refactor-kiln-test branch July 20, 2023 18:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants