Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

test: add unit testing for pdp package #43

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Conversation

frrist
Copy link
Member

@frrist frrist commented Feb 21, 2025

What

How to Review

Each commit is reviewable on it's own.

frrist and others added 3 commits February 21, 2025 12:03
Comment on lines +157 to +165
{
name: "Single piece >256MB (if code permits) => immediate flush or error",
// TODO(forrest): do we expect this to be an error case?
// The aggregator code comment suggests >256MB is out-of-scope, but not
// strictly enforced. Some implementations could treat this
// as an error or just flush.
pieceSizes: []int64{300 * MB},
expectedBufferSize: 0,
expectedAggCount: 1,
Copy link
Member Author

Choose a reason for hiding this comment

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

Unsure if this case is realistic, AggregatePieces states a pieces should never be larger than 256MB, but doesn't enforce the constraint.

Comment on lines +60 to +65
func WithAggregator(a BufferedAggregator) PieceAggregatorOption {
return func(pa *PieceAggregator) {
pa.aggregator = a
}
}

Copy link
Member Author

Choose a reason for hiding this comment

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

Allows the aggregator implementation to be subbed out for a mock in testing.

@@ -0,0 +1,166 @@
package aggregator_test
Copy link
Member Author

Choose a reason for hiding this comment

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

Seeking opinions on testing strategy here. So much of this is mocked that all we are really doing is asserting the right methods are called in the right order. While this add some value, my feeling is an integration/system test would be more useful here, ideally ad a higher level in the call stack.

@@ -15,6 +15,20 @@ import (
"github.com/storacha/go-ucanto/principal"
)

type PDPClient interface {
Copy link
Member Author

Choose a reason for hiding this comment

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

Defined interface for Curio client, allowing it to be mocked in curiofinder_test and curioadder_test.

@frrist frrist requested a review from hannahhoward February 21, 2025 20:14
@frrist frrist self-assigned this Feb 21, 2025
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

PDP Unit Tests
1 participant