Add unit tests for artifact store #554
Conversation
This PR adds unit tests for libartfact's store. It's only a first pass but should help finding any regressions. Speaking of which, this PR also fixes a regression that was introduced in artifact Add. The tests were basically lifted from my previous PR and isolated as such. Signed-off-by: Brent Baude <bbaude@redhat.com>
|
✅ A new PR has been created in buildah to vendor these changes: containers/buildah#6601 |
|
Test PR for podman -> containers/podman#27761 |
mtrmac
left a comment
mtrmac
left a comment
There was a problem hiding this comment.
Really just the .Replace implementation comment — on tests I’ll completely defer the style to people working on this subpackage.
| return nil, err | ||
| } | ||
| var err2 error | ||
| oldDigest, err2 = arty.GetDigest() |
There was a problem hiding this comment.
AFAICS oldDigest is later used for deleting the old version; if we already fully delete it here, isn’t that sufficient?
If this were required: It might work just fine as is — just, as a reader, reading metadata about an object after deleting it would require an extra look, and ordering it the other way would trivially remove that concern.
| // Create new manifest | ||
| annotations := make(map[string]string) | ||
| if options.Annotations != nil { | ||
| annotations = maps.Clone(options.Annotations) | ||
| } | ||
| annotations[specV1.AnnotationCreated] = time.Now().UTC().Format(time.RFC3339Nano) | ||
|
|
||
| artifactManifest = specV1.Manifest{ | ||
| Versioned: specs.Versioned{SchemaVersion: ManifestSchemaVersion}, | ||
| MediaType: specV1.MediaTypeImageManifest, | ||
| ArtifactType: options.ArtifactMIMEType, | ||
| Config: specV1.DescriptorEmptyJSON, | ||
| Layers: make([]specV1.Descriptor, 0), | ||
| Annotations: annotations, | ||
| } |
There was a problem hiding this comment.
Would it be worthwhile to share this with the default: case? This new version already dropped a comment, the are very likely to get of sync over time.
| // if no specific files were passed, create a random file of 2k | ||
| if fileNames == nil { | ||
| filename, err := randomAlphanumeric(5) | ||
| require.NoError(t, err) | ||
| fileNames = map[string]int{ | ||
| filename: 2, |
| // Verify artifact properties | ||
| artifact := artifacts[0] | ||
| assert.Equal(t, refName, artifact.Name) | ||
| assert.Equal(t, "application/vnd.test+type", artifact.Manifest.ArtifactType) |
There was a problem hiding this comment.
(This is sort of a remote dependency on helperAddArtifact’s default, but, as a test of the storage persisting data, it works well enough.
Also happens in other tests.
This doesn’t necessarily require making this more verbose in all helperAdd… calls — maybe define a constant for this value, and use it both in helperAdd… and in tests; that will demonstrate that the string values are related.)
| stat1, err := os.Stat(extractedFile1) | ||
| require.NoError(t, err) | ||
| assert.Equal(t, int64(512), stat1.Size()) | ||
|
|
||
| stat2, err := os.Stat(extractedFile2) | ||
| require.NoError(t, err) | ||
| assert.Equal(t, int64(1024), stat2.Size()) | ||
|
|
||
| stat3, err := os.Stat(extractedFile3) | ||
| require.NoError(t, err) | ||
| assert.Equal(t, int64(2048), stat3.Size()) | ||
|
|
There was a problem hiding this comment.
This is really redundant with the assert.Len checks below. (Absolutely non-blocking: Comparing the actual contents as well might be valuable.)
| } | ||
| } | ||
|
|
||
| func TestDetermineBlobMIMEType_FromFile(t *testing.T) { |
There was a problem hiding this comment.
(Absolutely non-blocking: This might be easier table-driven; in fact all 3 of the “success” test functions for determine… could share a test core, and either have separate tables, or be merged into one test function.)
|
|
||
| _, _, err = determineBlobMIMEType(blob) | ||
| require.Error(t, err) | ||
| assert.Contains(t, err.Error(), "Artifact.BlobFile or Artifact.BlobReader must be provided") |
There was a problem hiding this comment.
[When we are testing on exact error messages at all… but it’s useful in this case] the error message is maybe technically accurate, but not obviously matching the input.
3 participants
This PR adds unit tests for libartfact's store. It's only a first pass but should help finding any regressions. Speaking of which, this PR also fixes a regression that was introduced in artifact Add.
The tests were basically lifted from my previous PR and isolated as such.