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

#2815 do not broadcast heartbeat proposal when we are non-validator #2819

Merged
merged 12 commits into from Nov 17, 2018

Conversation

srmo
Copy link
Contributor

@srmo srmo commented Nov 12, 2018

Refs #2815

  • Updated all relevant documentation in docs
  • Updated all code comments where relevant
  • Wrote tests
  • Updated CHANGELOG_PENDING.md

@codecov-io
Copy link

codecov-io commented Nov 12, 2018

Codecov Report

Merging #2819 into develop will decrease coverage by 0.2%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           develop   #2819      +/-   ##
==========================================
- Coverage    62.51%   62.3%   -0.21%     
==========================================
  Files          212     212              
  Lines        17168   17221      +53     
==========================================
- Hits         10733   10730       -3     
- Misses        5539    5593      +54     
- Partials       896     898       +2
Impacted Files Coverage Δ
consensus/state.go 79.88% <100%> (-0.5%) ⬇️
consensus/reactor.go 66.86% <100%> (-2.45%) ⬇️
libs/db/remotedb/grpcdb/client.go 0% <0%> (ø) ⬆️
blockchain/pool.go 66.43% <0%> (+0.69%) ⬆️
libs/db/debug_db.go 20.63% <0%> (+4.63%) ⬆️
privval/ipc_server.go 69.81% <0%> (+5.66%) ⬆️

@srmo
Copy link
Contributor Author

srmo commented Nov 12, 2018

Sorry for the noisy cosmetic commits

@srmo
Copy link
Contributor Author

srmo commented Nov 12, 2018

Seems as if I'm not able to understand how to write correct tests in your framework.
I have a vague idea how to write the test that waits for the Event, but I would have no idea on how to write the negative test with your ensure methods.
Help appreciated.

- try and use "ensure" pattern in common_test
@srmo
Copy link
Contributor Author

srmo commented Nov 12, 2018

OK, fixed the positive case. The negative case still looks off.

Copy link
Contributor

@melekes melekes left a comment

Choose a reason for hiding this comment

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

🍓 🍌 🥛

consensus/common_test.go Outdated Show resolved Hide resolved
consensus/common_test.go Outdated Show resolved Hide resolved
consensus/state.go Show resolved Hide resolved
consensus/state_test.go Show resolved Hide resolved
consensus/state_test.go Outdated Show resolved Hide resolved
@srmo
Copy link
Contributor Author

srmo commented Nov 15, 2018

@melekes fixed merge conflict vs develop after 26.2-RC0 release

cs.Stop()

// if a faulty implementation sends an event, we should wait here a little bit to make sure we don't miss it by prematurely leaving the test method
time.Sleep((proposalHeartbeatIntervalSeconds + 1) * time.Second)
Copy link
Contributor

Choose a reason for hiding this comment

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

unfortunate 3 new seconds to wait in the test. But this whole thing will go away soon anyways.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm...I could try myself on removing this proposalHeartbeat infrastructure

@@ -1027,6 +1029,33 @@ func TestSetValidBlockOnDelayedPrevote(t *testing.T) {
assert.True(t, rs.ValidRound == round)
}

// regression for #2518
Copy link
Contributor

Choose a reason for hiding this comment

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

2815 ;)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oops :)

@ebuchman ebuchman merged commit 1466a2c into tendermint:develop Nov 17, 2018
@srmo srmo deleted the 2815-hearbeat-non-validator branch November 17, 2018 21:18
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

4 participants