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: add benchmarks and share commitment test #1726

Merged
merged 33 commits into from
May 12, 2023

Conversation

cmwaters
Copy link
Contributor

@cmwaters cmwaters commented May 9, 2023

Closes: #1717

@cmwaters cmwaters requested a review from evan-forbes as a code owner May 9, 2023 15:23
@MSevey MSevey requested review from a team and MSevey and removed request for a team May 9, 2023 15:23
@codecov-commenter
Copy link

codecov-commenter commented May 9, 2023

Codecov Report

Merging #1726 (9cd4101) into main (1b5263a) will decrease coverage by 0.11%.
The diff coverage is 11.76%.

@@            Coverage Diff             @@
##             main    #1726      +/-   ##
==========================================
- Coverage   52.05%   51.95%   -0.11%     
==========================================
  Files          93       93              
  Lines        6049     6065      +16     
==========================================
+ Hits         3149     3151       +2     
- Misses       2576     2586      +10     
- Partials      324      328       +4     
Impacted Files Coverage Δ
app/process_proposal.go 0.00% <0.00%> (ø)
pkg/square/builder.go 78.02% <12.50%> (-4.08%) ⬇️

rootulp
rootulp previously approved these changes May 9, 2023
dah := da.NewDataAvailabilityHeader(eds)
decoder := encoding.MakeConfig(app.ModuleEncodingRegisters...).TxConfig.TxDecoder()

for pfbIndex := 0; pfbIndex < 10; pfbIndex++ {
Copy link
Collaborator

Choose a reason for hiding this comment

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

[nit] it may help to extract 10 to a variable in this function so that the test doesn't break if line 370 is modified without a corresponding update on this line and the line below

Base automatically changed from cal/square-3 to main May 10, 2023 09:20
@cmwaters cmwaters requested a review from rach-id as a code owner May 10, 2023 09:20
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.

weird that github is showing the total diff before #1690

beyond the typical confict stuff I think this LGTM! its difficult to be certain because of the whole diff issues, but only TestSquareShareCommitments and the new benchmark are added here, correct?

}

func TestSquareShareCommitments(t *testing.T) {
txs := generateOrderedTxs(10, 10, 5)
Copy link
Member

@evan-forbes evan-forbes May 10, 2023

Choose a reason for hiding this comment

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

in the past, we've found edge case bugs in the share splitting process by running this same test over every square, effectively fuzzing it. Since this property is critical to celestia functioning as intended, I think it would be valuable to expand on the inputs to this test either here or in the future. I'm also happy to take this on, as not all follow ups to #1690 have to be handled immediatly or in this PR

@MSevey MSevey requested a review from a team May 11, 2023 08:48
@cmwaters cmwaters force-pushed the cal/test-share-commitments branch from 9831bdc to a2ecb34 Compare May 11, 2023 08:53
@cmwaters
Copy link
Contributor Author

Ok I think I've fixed the merge conflicts here. It is annoying that it can't resolve the diff properly. I think that might be because I made changes to the dependent branch before merging it to main so it doesn't see the common commit

@cmwaters cmwaters requested review from evan-forbes and removed request for MSevey and rach-id May 11, 2023 20:48
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.

thanks for following up on this!

@MSevey MSevey requested review from a team and rach-id and removed request for a team May 12, 2023 07:57
@cmwaters cmwaters merged commit 1b2532b into main May 12, 2023
@cmwaters cmwaters deleted the cal/test-share-commitments branch May 12, 2023 08:33
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.

Test share commitments match the commitments for the share ranges in the square.
4 participants