Fix and simplify storage tests #2147
Conversation
Well, so much for that part. To be continued. |
|
LGTM |
mtrmac
added
the
kind/bug
|
Changes LGTM, but there are a number of "Operation not Permited" errors. |
|
I think containers/storage#1735 might get us a bit closer, but ultimately I don’t think c/storage can run entirely unprivileged (without adding some more options throughout the codebase). So I currently plan to settle with making the tests be macOS-only (where ~everything is fake, due to containers/storage#811 ). They won’t run on CI, but at least frequently on my machine. |
containers#731 introduced a WaitGroup which is waited but never signaled, and a nonsensical error reporting mechanism (where errors are pushed but don't terminate future operations). Instead, rely on PipeWriter.CloseWithError() to propagate failures from the goroutine. And move all of the body of the goroutine into a separate function, so that we can use defer() to ensure CloseWithError() is _always_ called. Signed-off-by: Miloslav Trmač <mitr@redhat.com>
defer cleanup immediately after creating objects, use shorter error checking. Should not change behavior. Signed-off-by: Miloslav Trmač <mitr@redhat.com>
Signed-off-by: Miloslav Trmač <mitr@redhat.com>
The old ones are too fake and nowadays rejected when reading. Signed-off-by: Miloslav Trmač <mitr@redhat.com>
... so that we can update it in one place. Should not change (test) behavior, other than the precise skip message. Signed-off-by: Miloslav Trmač <mitr@redhat.com>
c/storage on macOS runs in a special mode which does not require any privileges. Signed-off-by: Miloslav Trmač <mitr@redhat.com>
Return a struct so that we can maintain an array of them. Create a helper to call PutBlob and format config data. Should not change (test) behavior. Signed-off-by: Miloslav Trmač <mitr@redhat.com>
Add createUncommittedImageDest / createImage which calls the ImageDestination APIs as required, so that test bodies can focus on the primary purpose. Should not change (test) behavior. Signed-off-by: Miloslav Trmač <mitr@redhat.com>
TestWriteRead could probably share more with createImage, but this just might do. Signed-off-by: Miloslav Trmač <mitr@redhat.com>
nil is defined to mean the same thing; if anything, nil is the value more likely to trigger crashes. Signed-off-by: Miloslav Trmač <mitr@redhat.com>
I have modified the PR to work that way. containers/storage#1735 would be cleaner but is not strictly required, so I have dropped the c/storage update requirement here. |
|
Ready for review/merging again. |
|
LGTM |
For CRI-O (and, maybe, Podman/libimate), I want to introduce a new reference lookup method, and that needs tests against a locally-created image.
It turns out those tests required root, and we never run them as root. So a large part of storage transport tests have been unused for 4 years now.
This fixes that problem:
Fixes #1729 . And at least on a Mac (?), the tests don’t actually require any privileges, so run them unconditionally.
This PR also consolidates the existing code to create/test images to significantly reduce duplication, to benefit from
testify, and to make it easier to add more test cases, now that I have been reading the code.@vrothberg PTAL. Cc: @saschagrunert