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

Refactor init_requester #152

Merged
merged 8 commits into from
Feb 11, 2020
Merged

Conversation

Shivani912
Copy link
Contributor

Updates:

  • refactored init_requester to take vector of commits (previously, it assumed all the Vals have signed)
  • updated existing tests for the new init_requester
  • added bisection test for insufficient commits
  • Referenced an issue explaining the need for the change
  • Updated all relevant documentation in docs
  • Updated all code comments where relevant
  • Wrote tests
  • Updated CHANGES.md

@xla xla changed the title Refactored init_requester Refactor init_requester Feb 11, 2020
@@ -377,15 +377,16 @@ mod tests {
// create an initial trusted state from the given vals
fn init_trusted_state(
vals_vec: Vec<usize>,
commit_vec: Vec<usize>,
Copy link
Member

Choose a reason for hiding this comment

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

Hmm, now that we use a pair ValsAndCommit below, should we use it here too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, will do that 👍

@@ -404,19 +405,32 @@ mod tests {
(MockSignedHeader::new(commit, header), vals, next_vals)
}


// TODO: find a better name
Copy link
Member

Choose a reason for hiding this comment

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

I think the name is actually OK (especially as this "just" test-code).

Copy link
Member

@liamsi liamsi left a comment

Choose a reason for hiding this comment

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

👍

@codecov-io
Copy link

Codecov Report

Merging #152 into ismail/132_141_followup will increase coverage by 0.86%.
The diff coverage is 90.99%.

Impacted file tree graph

@@                    Coverage Diff                     @@
##           ismail/132_141_followup    #152      +/-   ##
==========================================================
+ Coverage                    36.73%   37.6%   +0.86%     
==========================================================
  Files                           90      90              
  Lines                         3174    3231      +57     
  Branches                       497     497              
==========================================================
+ Hits                          1166    1215      +49     
- Misses                        1748    1756       +8     
  Partials                       260     260
Impacted Files Coverage Δ
tendermint/src/lite/verifier.rs 82.44% <90.99%> (+0.76%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6ce0262...3eb6cc1. Read the comment docs.

@liamsi liamsi merged commit f577dde into ismail/132_141_followup Feb 11, 2020
@liamsi liamsi deleted the shivani/bisection_test branch February 11, 2020 18:58
Shivani912 added a commit that referenced this pull request Feb 19, 2020
* Valid TrustThresholdFraction for real

* Further simplify tests: init requester with final state

* Apply review suggestions and assert_bisection_err fn

* remove redundant clone

* add another err case

* unpublicize verify_commit_full and verify_commit_trusting (ref #136)

* ensure the untrusted_header.bft_time() > trusted_header.bft_time()

* Fix failing test and test and add another erroring one

* remove dupl test

* Consistent comments in tests

* Refactor init_requester (#152)

* added bisection test for insufficient commits

* refactored init_requester, TODO: update tests

* fixed tests

* Added bisection test for insufficient commits

* fixed failures

* cargo fmt

* Changed tests to use ValsAndCommit struct

* satisfy clippy

* Dealing with the merge conflict's aftermath

Co-authored-by: Shivani Joshi <46731446+Shivani912@users.noreply.github.com>
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