Skip to content

Add unit tests for datasource.go#55

Merged
rosygmiki merged 1 commit into
containers:mainfrom
melkoudi-cmyk:TE-4763-datasource-ut
Jul 16, 2026
Merged

Add unit tests for datasource.go#55
rosygmiki merged 1 commit into
containers:mainfrom
melkoudi-cmyk:TE-4763-datasource-ut

Conversation

@melkoudi-cmyk

@melkoudi-cmyk melkoudi-cmyk commented Jul 8, 2026 •
edited
Loading

Copy link
Copy Markdown
Collaborator

This pr adds unit test coverage for the datasource.go implementation

Fixes: #23

Assisted by: Sonnet 4.5

@melkoudi-cmyk
melkoudi-cmyk force-pushed the TE-4763-datasource-ut branch 7 times, most recently from 4c0ba87 to c45d18f Compare July 8, 2026 14:28
arortiz-rh
arortiz-rh reviewed Jul 9, 2026
View reviewed changes

@arortiz-rh arortiz-rh left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Along with the changes I suggested, I also noticed that there aren't any tests checking for what happens if we try and read a file after a close is completed. This might be good coverage to include.

Comment thread pkg/oci-delta/datasource_test.go Outdated
Comment thread pkg/oci-delta/datasource_test.go
@arortiz-rh

arortiz-rh commented Jul 10, 2026

Copy link
Copy Markdown
Contributor

I was looking at the commit message and just realized that you said this fixes #26 which it should be fixing #23

knecasov
knecasov reviewed Jul 14, 2026
View reviewed changes

@knecasov knecasov left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added my comments. Please, consider the following.

  • Read/Seek without calling SetCurrentFile first: could be useful as an additional edge case test.

  • Error messages mix method names with parentheses and plain method names. I would recommend picking one style and keep it consistent across the file.

Comment thread pkg/oci-delta/datasource_test.go Outdated
Comment thread pkg/oci-delta/datasource_test.go Outdated
Comment thread pkg/oci-delta/datasource_test.go Outdated
Comment thread pkg/oci-delta/datasource_test.go Outdated
Comment thread pkg/oci-delta/datasource_test.go Outdated
Comment thread pkg/oci-delta/datasource_test.go Outdated
@melkoudi-cmyk
melkoudi-cmyk force-pushed the TE-4763-datasource-ut branch 4 times, most recently from 5e83c03 to 08bf779 Compare July 15, 2026 12:54
@rosygmiki
rosygmiki requested review from arortiz-rh and knecasov July 15, 2026 13:00
@melkoudi-cmyk
melkoudi-cmyk force-pushed the TE-4763-datasource-ut branch 2 times, most recently from 240ac1f to feb4693 Compare July 15, 2026 14:04
arortiz-rh
arortiz-rh suggested changes Jul 15, 2026
View reviewed changes

@arortiz-rh arortiz-rh left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All the changes I requested look good, the only thing that still needs to be done is changing the commit message to say it Fixes: #23 rather than Fixes: #26.

This can be done with just a quick rebase and choosing to reword the commit

rosygmiki
rosygmiki approved these changes Jul 16, 2026
View reviewed changes

@rosygmiki rosygmiki left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The test file ran successfully, and all reviewer comments have been addressed. LGTM.

@melkoudi-cmyk
melkoudi-cmyk force-pushed the TE-4763-datasource-ut branch from feb4693 to f94b791 Compare July 16, 2026 12:59
@melkoudi-cmyk
melkoudi-cmyk force-pushed the TE-4763-datasource-ut branch 2 times, most recently from ebdd5cf to c362393 Compare July 16, 2026 14:46
knecasov
knecasov reviewed Jul 16, 2026
View reviewed changes

@knecasov knecasov left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would add () to the function names as follows.
Read() %d bytes, want %d
Read() content = %q, want %q
Seek() position = %d, want 5

I would add for after () in the following comments.
Read() for nested file failed: %v
Read() for nested file = %q, want %q
Read() for empty file error = %v, want io.EOF
Read() for empty file returned %d bytes, want 0

@melkoudi-cmyk
melkoudi-cmyk force-pushed the TE-4763-datasource-ut branch from c362393 to 5075a8b Compare July 16, 2026 17:18
knecasov
knecasov reviewed Jul 16, 2026
View reviewed changes
Comment thread pkg/oci-delta/datasource_test.go Outdated
}

if n != len(testContent) {
t.Errorf("Read() %d bytes, want %d", n, len(testContent))

@knecasov knecasov Jul 16, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe the last thing from my side - could you edit the text as follows, please?
Read() returned %d bytes, want %d

@melkoudi-cmyk
Add unit tests for datasource.go
f3d6a76 
This pr adds unit test coverage for the datasource.go
implementation

Assisted by: Sonnet 4.5

Fixes: containers#23

Signed-off-by: Meriem Elkoudi <melkoudi@redhat.com>
@melkoudi-cmyk
melkoudi-cmyk force-pushed the TE-4763-datasource-ut branch from 5075a8b to f3d6a76 Compare July 16, 2026 17:34
knecasov
knecasov approved these changes Jul 16, 2026
View reviewed changes

@knecasov knecasov left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thank you very much for incorporating all my notes!

@rosygmiki
rosygmiki merged commit cf25345 into containers:main Jul 16, 2026
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@rosygmiki rosygmiki rosygmiki approved these changes
+2 more reviewers
@arortiz-rh arortiz-rh arortiz-rh approved these changes
@knecasov knecasov knecasov approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Add unit tests for datasource.go

4 participants

@melkoudi-cmyk @arortiz-rh @knecasov @rosygmiki