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.

Sign up for GitHub

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

Jump to bottom

Return value of setCell isn't checked #202

Closed
rootulp opened this issue Jun 30, 2023 · 1 comment · Fixed by #203
Closed

Return value of setCell isn't checked #202

rootulp opened this issue Jun 30, 2023 · 1 comment · Fixed by #203
Assignees
@rootulp
Labels
bug Something isn't working

Comments

@rootulp
Copy link
Collaborator

rootulp commented Jun 30, 2023 •
edited
Loading

Context

rsmt2d/extendeddatacrossword_test.go

Line 209 in 2557620

eds.setCell(x, y, test.values[i])

Problem

After #172 setCell may return an error and indeed does when trying to overwrite a cell with nil. This means an error is going undetected in the test.

Proposal

Option A

Revert setCell to not perform input validation.

Option B

Check the the return value of setCell in the test. Fail the test if an error is observed.

Follow ups

#204
@rootulp rootulp self-assigned this Jun 30, 2023
This was referenced Jun 30, 2023
Merged
Closed
@rootulp rootulp added the bug Something isn't working label Jun 30, 2023
@rootulp
Copy link
Collaborator Author

rootulp commented Jun 30, 2023 •
edited
Loading

Based on the tests in extendeddatacrossword_test.go, the desired behavior is that setCell is able to overwrite cells with nil (see here)so going with Option A.

rootulp added a commit that referenced this issue Jul 4, 2023
@rootulp
test: do not perform input validation in setCell (#203)
ed26fa9 
Closes #202 via **Option A**

Note: this isn't breaking b/c `setCell` isn't exported.
0xchainlover pushed a commit to celestia-org/rsmt2d that referenced this issue Aug 1, 2024
@rootulp
test: do not perform input validation in setCell (#203)
4800c51 
Closes celestiaorg/rsmt2d#202 via **Option A**

Note: this isn't breaking b/c `setCell` isn't exported.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@rootulp rootulp

Labels
bug Something isn't working
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

test: do not perform input validation in setCell rootulp/rsmt2d
1 participant
@rootulp