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

Conversation

rootulp
Copy link
Collaborator

@rootulp rootulp commented Jun 30, 2023

Closes #202 via Option A

Note: this isn't breaking b/c setCell isn't exported.

@rootulp rootulp requested review from staheri14 and evan-forbes June 30, 2023 18:00
@rootulp rootulp self-assigned this Jun 30, 2023
@codecov
Copy link

codecov bot commented Jun 30, 2023

Codecov Report

Merging #203 (173f4b6) into master (2557620) will increase coverage by 0.13%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #203      +/-   ##
==========================================
+ Coverage   80.80%   80.93%   +0.13%     
==========================================
  Files           7        7              
  Lines         500      493       -7     
==========================================
- Hits          404      399       -5     
+ Misses         57       56       -1     
+ Partials       39       38       -1     
Impacted Files Coverage Δ
datasquare.go 95.06% <ø> (+0.97%) ⬆️
extendeddatacrossword.go 70.94% <100.00%> (ø)

@rootulp rootulp added the bug Something isn't working label Jun 30, 2023
datasquare.go Outdated Show resolved Hide resolved
@rootulp rootulp requested a review from staheri14 July 3, 2023 17:54
Copy link
Contributor

@staheri14 staheri14 left a comment

Choose a reason for hiding this comment

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

Thanks for addressing the comments, LGTM!

@rootulp rootulp changed the title revert: do not perform input validation in setCell test: do not perform input validation in setCell Jul 3, 2023
@rootulp rootulp added the testing label Jul 3, 2023
Comment on lines +98 to 100
// Test_setCell verifies that setCell can overwrite cells without performing any
// input validation.
func Test_setCell(t *testing.T) {
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

Copy link
Member

@evan-forbes evan-forbes left a comment

Choose a reason for hiding this comment

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

makes sense LGTM

@rootulp rootulp merged commit ed26fa9 into celestiaorg:master Jul 4, 2023
@rootulp rootulp deleted the rp/revert-setCell branch July 4, 2023 14:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Return value of setCell isn't checked
3 participants