Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

test: do not perform input validation in setCell #203

Merged
merged 3 commits into from
Jul 4, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 0 additions & 12 deletions datasquare.go
Original file line number Diff line number Diff line change
Expand Up @@ -307,18 +307,6 @@ func (ds *dataSquare) SetCell(x uint, y uint, newChunk []byte) error {
return nil
}

// setCell sets a specific cell. setCell will overwrite any existing value.
// Returns an error if the newChunk is not the correct size.
func (ds *dataSquare) setCell(x uint, y uint, newChunk []byte) error {
if len(newChunk) != int(ds.chunkSize) {
return fmt.Errorf("cannot set cell with chunk size %d because dataSquare chunk size is %d", len(newChunk), ds.chunkSize)
}
ds.squareRow[x][y] = newChunk
ds.squareCol[y][x] = newChunk
ds.resetRoots()
return nil
}

// Flattened returns the concatenated rows of the data square.
func (ds *dataSquare) Flattened() [][]byte {
flattened := [][]byte(nil)
Expand Down
50 changes: 26 additions & 24 deletions datasquare_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -95,47 +95,40 @@ func TestSetCell(t *testing.T) {
}
}

// Test_setCell verifies that setCell can overwrite cells without performing any
// input validation.
func Test_setCell(t *testing.T) {
Comment on lines +98 to 100
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

on re-review it seems overkill to have a test for an un-exported function that only exists for the sake of testing so I can remove this unit test if reviewers request

type testCase struct {
name string
originalCell []byte
newCell []byte
wantErr bool
name string
original []byte
new []byte
}

testCases := []testCase{
{
name: "can set cell if originally nil",
originalCell: nil,
newCell: []byte{42},
wantErr: false,
name: "can set cell if originally nil",
original: nil,
new: []byte{42},
},
{
name: "can set cell if originally some value",
originalCell: []byte{1},
newCell: []byte{42},
wantErr: false,
name: "can set cell if originally some value",
original: []byte{1},
new: []byte{42},
},
{
name: "expect error if new cell is not the correct chunk size",
originalCell: nil,
newCell: []byte{1, 2}, // incorrect chunk size
wantErr: true,
name: "can set cell to nil",
original: []byte{1},
new: nil,
},
}

for _, tc := range testCases {
t.Run(tc.name, func(t *testing.T) {
ds, err := newDataSquare([][]byte{tc.originalCell, {2}, {3}, {4}}, NewDefaultTree)
ds, err := newDataSquare([][]byte{tc.original, {2}, {3}, {4}}, NewDefaultTree)
assert.NoError(t, err)

err = ds.setCell(0, 0, tc.newCell)
if tc.wantErr {
assert.Error(t, err)
} else {
assert.NoError(t, err)
assert.Equal(t, tc.newCell, ds.GetCell(0, 0))
}
ds.setCell(0, 0, tc.new)
assert.Equal(t, tc.new, ds.GetCell(0, 0))
})
}
}
Expand Down Expand Up @@ -468,3 +461,12 @@ func (d *errorTree) Push(data []byte) error {
func (d *errorTree) Root() ([]byte, error) {
return nil, fmt.Errorf("error")
}

// setCell overwrites the contents of a specific cell. setCell does not perform
// any input validation so most use cases should use `SetCell` instead of
// `setCell`. This method exists strictly for testing.
func (ds *dataSquare) setCell(x uint, y uint, newChunk []byte) {
ds.squareRow[x][y] = newChunk
ds.squareCol[y][x] = newChunk
ds.resetRoots()
}
4 changes: 2 additions & 2 deletions extendeddatacrossword.go
Original file line number Diff line number Diff line change
Expand Up @@ -171,7 +171,7 @@ func (eds *ExtendedDataSquare) solveCrosswordRow(

// Insert rebuilt shares into square.
for c, s := range rebuiltShares {
eds.setCell(uint(r), uint(c), s)
eds.SetCell(uint(r), uint(c), s)
}

return true, true, nil
Expand Down Expand Up @@ -235,7 +235,7 @@ func (eds *ExtendedDataSquare) solveCrosswordCol(

// Insert rebuilt shares into square.
for r, s := range rebuiltShares {
eds.setCell(uint(r), uint(c), s)
eds.SetCell(uint(r), uint(c), s)
}

return true, true, nil
Expand Down