-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Integration test refactoring #126
Integration test refactoring #126
Conversation
integration/integration_test.go
Outdated
t.FailNow() | ||
} | ||
|
||
_, ex, _, _ := runtime.Caller(0) |
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 a bit weird, could you use os.Args[0] instead?
https://golang.org/pkg/os/
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.
os.Args gives me where the test file is actually being run, which is in a tmp folder somewhere. I actually want to access the directory where the go file actually lives, which is what runtime.Caller gives.
integration/integration_test.go
Outdated
cwd := filepath.Dir(ex) | ||
|
||
// Grab the latest container-diff binary | ||
getContainerDiff := exec.Command("gsutil", "cp", "gs://container-diff/latest/container-diff-linux-amd64", ".") |
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.
Let's just require that container-diff is on the path.
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.
Done
integration/integration_test.go
Outdated
} | ||
|
||
|
||
for _, dockerfile := range dockerfiles { |
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.
filepath.Glob might be easier than ls'ing everything and filtering 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.
yep. done.
integration/integration_test.go
Outdated
"-c", buildContextPath, | ||
) | ||
|
||
err = kanikoCmd.Run() |
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.
nit: a small helper that takes the cmd and t and does this could be useful.
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.
done.
Looks like we'll have to move the container-diff installation piece to the kokoro setup (or the container that will run in GCB) |
This is ready for a look. |
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.
A few nits. Nice job.
integration/integration_test.go
Outdated
} | ||
|
||
// Make sure container-diff is on user's PATH | ||
err = exec.Command("container-diff").Run() |
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.
nit: exec.LookPath would probably be a bit easier 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.
Done
integration/integration_test.go
Outdated
for _, dockerfile := range dockerfiles { | ||
t.Run("test_"+dockerfile, func(t *testing.T) { | ||
dockerfile = dockerfile[len("dockerfile/")+1:] | ||
fmt.Printf("%s\n", dockerfile) |
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.
nit: t.Log here instead of fmt.Print
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.
Done
integration/integration_test.go
Outdated
|
||
// container-diff | ||
daemonDockerImage := daemonPrefix + dockerImage | ||
//daemonKanikoImage := daemonPrefix + kanikoImage |
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.
remove this
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.
Done
No description provided.