test: add coverage for display content and json handlers#1950
test: add coverage for display content and json handlers#1950bimakw wants to merge 1 commit intooras-project:mainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds initial test coverage for three display package files that previously had 0% coverage: discard.go, push.go, and pull.go in the display metadata/content packages.
Key changes:
- Adds unit tests for DiscardHandler methods (OnContentFetched, OnContentCreated) and interface compliance
- Adds unit tests for JSON PushHandler methods (OnTagged, OnCopied, Render) covering both tag and digest reference scenarios
- Adds unit tests for JSON PullHandler methods (OnLayerSkipped, OnFilePulled, OnPulled, Render)
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| cmd/oras/internal/display/metadata/json/push_test.go | Adds comprehensive tests for PushHandler including constructor, event handlers (OnTagged, OnCopied), and JSON rendering with both tag and digest references |
| cmd/oras/internal/display/metadata/json/pull_test.go | Adds tests for PullHandler covering constructor, layer skipping, file pulling, and JSON output rendering |
| cmd/oras/internal/display/content/discard_test.go | Adds tests for DiscardHandler verifying no-op behavior and interface compliance |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Verify JSON output is valid | ||
| var result map[string]interface{} | ||
| if err := json.Unmarshal(buf.Bytes(), &result); err != nil { | ||
| t.Errorf("PushHandler.Render() produced invalid JSON: %v", err) | ||
| } |
There was a problem hiding this comment.
The test validates that the output is valid JSON but doesn't verify the actual content or structure of the rendered output. Consider adding assertions to check for expected fields such as the digest, mediaType, path, and tags in the result to ensure the Render method produces the correct output structure.
| // Verify JSON output is valid | ||
| var result map[string]interface{} | ||
| if err := json.Unmarshal(buf.Bytes(), &result); err != nil { | ||
| t.Errorf("PullHandler.Render() produced invalid JSON: %v", err) | ||
| } |
There was a problem hiding this comment.
The test validates that the output is valid JSON but doesn't verify the actual content or structure of the rendered output. Consider adding assertions to check for expected fields such as the digest, path, and files array in the result to ensure the Render method produces the correct output structure.
Add test coverage for: - content/discard.go: DiscardHandler methods - metadata/json/push.go: PushHandler methods - metadata/json/pull.go: PullHandler methods Partial: oras-project#1916 Signed-off-by: Gregorius Bima Kharisma Wicaksana <51526537+bimakw@users.noreply.github.com>
6e2f273 to
cbe3cf2
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| func TestPushHandler_OnCopied(t *testing.T) { | ||
| buf := &bytes.Buffer{} | ||
| handler := NewPushHandler(buf).(*PushHandler) | ||
|
|
||
| desc := ocispec.Descriptor{ | ||
| MediaType: "application/vnd.oci.image.manifest.v1+json", | ||
| Digest: "sha256:e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855", | ||
| Size: 100, | ||
| } | ||
|
|
||
| opts := &option.Target{ | ||
| RawReference: "localhost:5000/test:v1.0.0", | ||
| Path: "localhost:5000/test", | ||
| Reference: "v1.0.0", | ||
| } | ||
|
|
||
| err := handler.OnCopied(opts, desc) | ||
| if err != nil { | ||
| t.Errorf("PushHandler.OnCopied() error = %v, want nil", err) | ||
| } | ||
| } |
There was a problem hiding this comment.
The test only verifies that OnCopied returns no error, but doesn't verify that the descriptor and options were actually stored in the handler's internal state. Consider adding assertions to check that handler.root equals the descriptor and handler.path equals opts.Path after calling OnCopied.
| func TestPushHandler_OnCopied_WithDigest(t *testing.T) { | ||
| buf := &bytes.Buffer{} | ||
| handler := NewPushHandler(buf).(*PushHandler) | ||
|
|
||
| desc := ocispec.Descriptor{ | ||
| MediaType: "application/vnd.oci.image.manifest.v1+json", | ||
| Digest: "sha256:e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855", | ||
| Size: 100, | ||
| } | ||
|
|
||
| opts := &option.Target{ | ||
| RawReference: "localhost:5000/test@sha256:e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855", | ||
| Path: "localhost:5000/test", | ||
| Reference: "sha256:e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855", | ||
| } | ||
|
|
||
| err := handler.OnCopied(opts, desc) | ||
| if err != nil { | ||
| t.Errorf("PushHandler.OnCopied() error = %v, want nil", err) | ||
| } | ||
| } |
There was a problem hiding this comment.
The test only verifies that OnCopied returns no error, but doesn't verify that the tag was NOT added to the handler's internal state when the reference is a digest. According to the OnCopied implementation, tags should only be added when the reference is not a digest. Consider adding assertions to verify that no tag was added in this digest-based reference scenario.
| func TestPullHandler_OnLayerSkipped(t *testing.T) { | ||
| buf := &bytes.Buffer{} | ||
| handler := NewPullHandler(buf, "localhost:5000/test").(*PullHandler) | ||
|
|
||
| desc := ocispec.Descriptor{ | ||
| MediaType: "application/vnd.oci.image.layer.v1.tar+gzip", | ||
| Digest: "sha256:e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855", | ||
| Size: 1024, | ||
| } | ||
|
|
||
| err := handler.OnLayerSkipped(desc) | ||
| if err != nil { | ||
| t.Errorf("PullHandler.OnLayerSkipped() error = %v, want nil", err) | ||
| } | ||
| } |
There was a problem hiding this comment.
The test only verifies that OnLayerSkipped returns no error, but since this is a no-op method that always returns nil, this test provides minimal value. Consider whether this test is necessary, or if it should be testing integration with other methods to ensure the layer being skipped doesn't affect the final output.
| func TestPullHandler_OnFilePulled(t *testing.T) { | ||
| buf := &bytes.Buffer{} | ||
| handler := NewPullHandler(buf, "localhost:5000/test").(*PullHandler) | ||
|
|
||
| desc := ocispec.Descriptor{ | ||
| MediaType: "application/vnd.oci.image.layer.v1.tar+gzip", | ||
| Digest: "sha256:e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855", | ||
| Size: 1024, | ||
| } | ||
|
|
||
| err := handler.OnFilePulled("test.txt", "/output", desc, "blobs/sha256/e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855") | ||
| if err != nil { | ||
| t.Errorf("PullHandler.OnFilePulled() error = %v, want nil", err) | ||
| } | ||
| } |
There was a problem hiding this comment.
The test only verifies that OnFilePulled returns no error, but doesn't verify that the file information was actually added to the handler's internal state. Consider adding assertions to check that the file was properly recorded in the pulled files list after calling OnFilePulled.
| func TestPushHandler_OnTagged(t *testing.T) { | ||
| buf := &bytes.Buffer{} | ||
| handler := NewPushHandler(buf).(*PushHandler) | ||
|
|
||
| desc := ocispec.Descriptor{ | ||
| MediaType: "application/vnd.oci.image.manifest.v1+json", | ||
| Digest: "sha256:e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855", | ||
| Size: 100, | ||
| } | ||
|
|
||
| err := handler.OnTagged(desc, "v1.0.0") | ||
| if err != nil { | ||
| t.Errorf("PushHandler.OnTagged() error = %v, want nil", err) | ||
| } | ||
| } |
There was a problem hiding this comment.
The test only verifies that OnTagged returns no error, but doesn't verify that the tag was actually added to the handler's internal state. Consider adding assertions to check that the tag is properly stored by verifying it appears in the handler's tags list after calling OnTagged.
|
This PR is stale because it has been open 45 days with no activity. Remove stale label or comment or this will be closed in 30 days. |
|
still working on this, will address the review comments soon |
What this PR does / why we need it:
Add test coverage for display packages that currently have 0% coverage.
Files covered:
content/discard.go: Tests for DiscardHandler methods (OnContentFetched, OnContentCreated)metadata/json/push.go: Tests for PushHandler methods (OnTagged, OnCopied, Render)metadata/json/pull.go: Tests for PullHandler methods (OnLayerSkipped, OnFilePulled, OnPulled, Render)Which issue(s) this PR fixes:
Partial: #1916
Please check the following list: