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

chore: define OriginalSquareSize inside generateRandomBlockData #717

Merged
merged 2 commits into from
Sep 21, 2022

Conversation

rootulp
Copy link
Collaborator

@rootulp rootulp commented Sep 16, 2022

Spun out of #691

Motivation

While iterating on the TestMerge test, I encountered an unexpected error

            	            	square size is not a power of two: 0

because I failed to declare the square size on the data returned from generateRandomBlockData. I moved a variable definition to avoid this scenario.

Very minor improvement. Can be closed if not helpful.

@rootulp rootulp self-assigned this Sep 16, 2022
@rootulp rootulp changed the title Define OriginalSquareSize inside generateRandomBlockData chore: define OriginalSquareSize inside generateRandomBlockData Sep 16, 2022
@codecov-commenter
Copy link

Codecov Report

Merging #717 (f12b6db) into main (7bbb270) will increase coverage by 1.69%.
The diff coverage is 64.60%.

@@            Coverage Diff             @@
##             main     #717      +/-   ##
==========================================
+ Coverage   44.52%   46.22%   +1.69%     
==========================================
  Files          32       33       +1     
  Lines        3189     3217      +28     
==========================================
+ Hits         1420     1487      +67     
+ Misses       1666     1619      -47     
- Partials      103      111       +8     
Impacted Files Coverage Δ
pkg/prove/querier.go 0.00% <0.00%> (ø)
pkg/shares/utils.go 4.08% <0.00%> (+4.08%) ⬆️
pkg/prove/proof.go 82.48% <73.68%> (-1.35%) ⬇️
pkg/shares/split_compact_shares.go 84.00% <77.77%> (+13.59%) ⬆️
pkg/shares/share_merging.go 70.78% <80.00%> (ø)
pkg/shares/split_sparse_shares.go 69.51% <90.00%> (ø)
pkg/da/data_availability_header.go 78.68% <100.00%> (ø)
pkg/shares/parse_compact_shares.go 77.77% <100.00%> (ø)
pkg/shares/parse_sparse_shares.go 88.60% <100.00%> (ø)
pkg/shares/share_splitting.go 60.24% <100.00%> (+46.98%) ⬆️
... and 9 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

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.

we can merge even it only fixes tests temporarily, I think I had to do this somewhere else as but forget where.

ideally we calculate the square size, but that's impossible in that package. might have to move it to a specific test package after #692 if we really want to test it.

@evan-forbes
Copy link
Member

will investigate this more tmrw

@rootulp
Copy link
Collaborator Author

rootulp commented Sep 16, 2022

we can merge even it only fixes tests temporarily

I don't need this change for tests to pass so this is strictly optional and can be closed if undesired

@rootulp rootulp enabled auto-merge (squash) September 16, 2022 16:23
@rootulp
Copy link
Collaborator Author

rootulp commented Sep 21, 2022

@evan-forbes do we still want this? If not, I'll close.

@rootulp rootulp merged commit 68b25ce into celestiaorg:main Sep 21, 2022
@evan-forbes
Copy link
Member

no reason not to I don't think

@evan-forbes
Copy link
Member

we might have to revert or override the square size if we don't want the max each time, but that's nbd

@rootulp rootulp deleted the rp/shares-test branch September 21, 2022 17:37
rach-id pushed a commit to rach-id/celestia-app that referenced this pull request Nov 16, 2022
…elestiaorg#717)

* test: specify square size

* move location of square size declaration
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants