Add analyzer acceptance tests #319
Conversation
natalieparellano
force-pushed
the
analyzer-acceptance
branch
from
485003f to
8f9f0e1
Compare
natalieparellano
force-pushed
the
analyzer-acceptance
branch
2 times, most recently
from
2d2f155 to
ffcaee8
Compare
| output := h.Run(t, cmd) | ||
|
|
||
| h.AssertMatch(t, output, "2222 3333 .+ \\.") | ||
| h.AssertMatch(t, output, "2222 3333 .+ committed") |
There was a problem hiding this comment.
The staging directory is not chowned - is that expected?
There was a problem hiding this comment.
It looks like the staging directory doesn't yet exist in the relevant cache fixture? If you are observing a staging dir with the wrong perms it is being created with the wrong perms which seems like an issue worth investigating.
There was a problem hiding this comment.
It looks like it is being created with the wrong permissions:
/cache:
total 4
drwxrwxr-x 4 2222 3333 128 Jun 26 19:59 .
drwxr-xr-x 1 root root 4096 Jun 26 19:59 ..
drwxrwxr-x 4 2222 3333 128 Aug 30 1754 committed
drwxr-xr-x 2 root root 64 Jun 26 19:59 staging
What's interesting, when I add a staging directory to my fixture, it looks like that is ignored? At least, I don't see the foo.txt file that I inserted there. Will investigate...
There was a problem hiding this comment.
Uggh I was probably doing something wrong - seeing different results now.
There was a problem hiding this comment.
I still see that foo.txt is ignored, but staging has the correct permissions. I am not sure what changed other than the cache directory is now being provided as a volume vs. bind mount. What is interesting is that in the case where the user's group can write, I see that staging is chowned:
/cache:
total 16
drwxrwxr-x 4 9999 3333 4096 Jun 29 20:23 .
drwxr-xr-x 1 root root 4096 Jun 29 20:23 ..
drwxrwxr-x 2 9999 3333 4096 Jun 29 20:21 committed
drwxr-xr-x 2 2222 3333 4096 Jun 29 20:23 staging
Adding a note to investigate further. There is some weirdness here that I'm not understanding.
There was a problem hiding this comment.
Looking at the code, if cacheDir != "" we always call cache.NewVolumeCache(cacheDir) which always recreates the staging directory (hence foo.txt was ignored). This happens after we drop privileges, so it makes sense that the uid & gid are 2222 & 3333. I have no idea why it was showing as owned by root before :/
There was a problem hiding this comment.
Alright, let's keep an eye on it.
natalieparellano
force-pushed
the
analyzer-acceptance
branch
from
ffcaee8 to
58f375b
Compare
natalieparellano
force-pushed
the
analyzer-acceptance
branch
3 times, most recently
from
1342e05 to
3e2286f
Compare
There was a problem hiding this comment.
Overall this looks awesome!
At first I was going to suggest asserting on file contents but given that we do this extensively in the unit tests I think assert on file existence was the right call. It keeps these tests simple and should catch high level workflow errors w/o becoming unmaintainable.
There was a problem hiding this comment.
In general, looks really cool! As @jromero mentioned, the use of explicit bash commands made it a bit hard for me as a relative lifecycle outsider to understand what was happening, so it may benefit by having a clearer wrapper func to make things more explicit.
Great job adding it!
natalieparellano
force-pushed
the
analyzer-acceptance
branch
21 times, most recently
from
8a64127 to
76b46e3
Compare
- Reorganize test helpers - Add small cross platform helpers (like variables) Signed-off-by: Natalie Arellano <narellano@vmware.com>
natalieparellano
force-pushed
the
analyzer-acceptance
branch
from
76b46e3 to
3480edd
Compare
|
@ekcasey I think this is ready for another round of review. |
There was a problem hiding this comment.
I am super pleased, this looks great!
I left a couple comments and questions but the only change I think is important to make before merging is the -cache-image+-daemon case. I am under the impression that combo should work. If for some reason it doesn't work, leave a test that asserts that it should work skipped or commented out and file a bug.
testhelpers/binaries.go
Outdated
| "testing" | ||
| ) | ||
|
|
||
| func MakeAndCopyLauncher(t *testing.T, destDir string) { |
There was a problem hiding this comment.
Should this take goos as an arg like MakeAndCopyLifecycle does?
There was a problem hiding this comment.
Changing it to be consistent. FWIW the reason this didn't take a goos is because in practice when we're testing the launcher it's always either Linux or Windows, so runtime.GOOS + some logic to convert darwin -> linux is sufficient. For the lifecycle, because we test the version on darwin the function needed more flexibility. What I have now is explicitly passing daemon OS as the goos for the launcher.
| func DockerCli(t *testing.T) dockercli.CommonAPIClient { | ||
| dockerCliOnce.Do(func() { | ||
| var dockerCliErr error | ||
| dockerCliVal, dockerCliErr = dockercli.NewClientWithOpts(dockercli.FromEnv, dockercli.WithVersion("1.38")) | ||
| AssertNil(t, dockerCliErr) | ||
| }) | ||
| return dockerCliVal | ||
| } | ||
|
|
||
| func DockerBuild(t *testing.T, name, context string, ops ...DockerCmdOp) { | ||
| t.Helper() | ||
| args := formatArgs([]string{"-t", name, context}, ops...) | ||
| Run(t, exec.Command("docker", append([]string{"build"}, args...)...)) | ||
| } |
There was a problem hiding this comment.
I am fine leaving this as is for now (I think I introduce this inconsistency in the launcher acceptance tests), but in the future we should decide whether we want to exec the docker cli or use the client library and be consistent about it (I lean towards the latter).
There was a problem hiding this comment.
I agree! I added a TODO for it in the code. Hopefully it will become consistent over time.
| analyzeImage, | ||
| "/layers", | ||
| h.WithFlags( | ||
| "--env", "CNB_REGISTRY_AUTH={}", |
There was a problem hiding this comment.
Why do we bother to pass the empty CNB_REGISTRY_AUTH in places where we don't use it?
There was a problem hiding this comment.
nvmd, comment on line 280 was illuminating. We should fix whatever causes that failure in the future.
There was a problem hiding this comment.
Opened an issue: #339
| output := h.Run(t, cmd) | ||
|
|
||
| h.AssertMatch(t, output, "2222 3333 .+ \\.") | ||
| h.AssertMatch(t, output, "2222 3333 .+ committed") |
There was a problem hiding this comment.
Alright, let's keep an eye on it.
| }) | ||
| }) | ||
|
|
||
| when("the provided cache directory is writeable by the CNB user's group", func() { |
There was a problem hiding this comment.
Having some test coverage for this logic makes me very happy :)
Signed-off-by: Natalie Arellano <narellano@vmware.com>
Signed-off-by: Natalie Arellano narellano@vmware.com