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

consensus: add comment as to why use mocks during replay #4785

Merged
merged 7 commits into from
May 6, 2020

Conversation

melekes
Copy link
Contributor

@melekes melekes commented May 4, 2020

Closes #4766

@melekes melekes requested a review from tessr as a code owner May 4, 2020 07:20
@auto-comment
Copy link

auto-comment bot commented May 4, 2020

👋 Thanks for creating a PR!

Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.

  • Wrote tests
  • Updated CHANGELOG_PENDING.md
  • Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
  • Updated relevant documentation (docs/) and code comments
  • Re-reviewed Files changed in the Github PR explorer
  • Applied Appropriate Labels

Thank you for your contribution to Tendermint! 🚀

@melekes melekes self-assigned this May 4, 2020
@melekes melekes added C:docs Component: Documentation S:automerge Automatically merge PR when requirements pass labels May 4, 2020
@codecov-io
Copy link

Codecov Report

❗ No coverage uploaded for pull request base (master@336b929). Click here to learn what that means.
The diff coverage is n/a.

@@            Coverage Diff            @@
##             master    #4785   +/-   ##
=========================================
  Coverage          ?   64.87%           
=========================================
  Files             ?      241           
  Lines             ?    23074           
  Branches          ?        0           
=========================================
  Hits              ?    14970           
  Misses            ?     6888           
  Partials          ?     1216           
Impacted Files Coverage Δ
consensus/replay.go 73.64% <ø> (ø)

consensus/replay.go Outdated Show resolved Hide resolved
@erikgrinaker
Copy link
Contributor

Is it much more expensive to set up an empty mempool and memory-backed evidence pool instead, or simply pass nil?

@melekes
Copy link
Contributor Author

melekes commented May 5, 2020

Is it much more expensive to set up an empty mempool and memory-backed evidence pool instead, or simply pass nil?

nil does not satisfy the interface. memory backed evidence pool - we would have to create one. evmock is pretty much the same.

to avoid future conflicts and freeze the required behaviour (nope stubs)
@erikgrinaker
Copy link
Contributor

erikgrinaker commented May 5, 2020

nil does not satisfy the interface.

nil satisfies all interfaces.

memory backed evidence pool - we would have to create one. evmock is pretty much the same.

I believe @cmwaters renamed it to emptyEvidencePool or something, which I guess is slightly better.

@melekes
Copy link
Contributor Author

melekes commented May 5, 2020

nil satisfies all interfaces.

right, I forgot it. Thanks!

@melekes
Copy link
Contributor Author

melekes commented May 5, 2020

nil does not work because we call methods on mempoo/evidence pool.

@erikgrinaker
Copy link
Contributor

nil does not work because we call methods on mempoo/evidence pool.

So check if it's nil? If there's a need to run this without mempool/evidence pool, then let's just make them optional?

@melekes
Copy link
Contributor Author

melekes commented May 5, 2020

So check if it's nil? If there's a need to run this without mempool/evidence pool, then let's just make them optional?

too many if conditions?

@melekes melekes added the R:minor Release: Minor label May 6, 2020
@melekes melekes merged commit f6435f2 into master May 6, 2020
@melekes melekes deleted the anton/4766-replay branch May 6, 2020 06:32
@tessr tessr removed the R:minor Release: Minor label May 27, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C:docs Component: Documentation S:automerge Automatically merge PR when requirements pass
Projects
None yet
Development

Successfully merging this pull request may close these issues.

why use the mock.Mempool{} to initialize the blockExec
5 participants