-
Notifications
You must be signed in to change notification settings - Fork 103
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
Add documentation for KUDO integration tests and move into test/integration. #490
Add documentation for KUDO integration tests and move into test/integration. #490
Conversation
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.
Strongly support. Willing to approve but I would really like a few questions addressed.
nice work!
pkg/test/case_test.go
Outdated
@@ -20,7 +20,7 @@ func TestLoadTestSteps(t *testing.T) { | |||
testSteps []Step | |||
}{ | |||
{ | |||
"test_data/with-overrides/", | |||
"../../test/integration/with-overrides/", |
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 strongly support moving testing to the test
folder from root. This is my least favorite part of it. "../../test". so every test we will need to know how deep we are in the package structure and every time we refactor packages we will need to deal with this? It seems easy for a human to fat finger. Is there anything we can think of to solve this in a better way?
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.
This is just one or two cases that exercise various parts of the test suite.
So, two ideas:
- We leave it with the
../../test/integration/with-overrides/
. This would only really break if someone changed thewith-overrides
test for some reason. - We leave the tests that
case_test.go
inspects inpkg/test/test_data
since they are literally that, just test data.
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 think I'll go with option two. Especially since they aren't testing KUDO, just the harness.
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.
Fixed!
@@ -15,10 +15,12 @@ import ( | |||
// If testToRun is set to a non-empty string, it is passed as a `-run` argument to the go test harness. | |||
func RunTests(testName string, testToRun string, testFunc func(*testing.T)) { | |||
// Set the verbose test flag to true since we are not using the regular go test CLI. | |||
flag.Lookup("test.v").Value.Set("true") | |||
flag.Set("test.v", "true") |
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.
why the switch here?
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 just noticed when I opened the docs that they had a helper method for it already, so cleaned it up
What type of PR is this?
/kind cleanup
/kind documentation
What this PR does / why we need it:
Moves the integration tests into
test/integration
and improves the documentation.Which issue(s) this PR fixes:
Fixes #469
Does this PR introduce a user-facing change?: