-
Notifications
You must be signed in to change notification settings - Fork 19
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
Conversation
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. |
kiln test
to use a domain packagekiln test
to use package encapsulating containerized tile test functionality
kiln test
to use package encapsulating containerized tile test functionalitykiln test
to use a function encapsulating containerized tile test functionality
kiln test
to use a function encapsulating containerized tile test functionality70486e9
to
bb85f96
Compare
53c269c
to
c32ef8a
Compare
There was a problem hiding this 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!
521f9e9
to
a66923d
Compare
d27d607
to
a3ea6c9
Compare
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
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.
a3ea6c9
to
0ca39d2
Compare
This change introduces a new private package called "test" (full name "github.com/pivotal-cf/kiln/internal/test"). It exports two identifiers.
test.Run
andtest.Configuration
. The former is a function with signature:The Configuration structure is defined as:
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 rungo test -short ./...
from the project root and it finishes in a couple seconds.