Skip to content

Conversation

@theStack
Copy link
Contributor

While working on the test for #19776 I noticed that creating a sendcmpct message is quite cumbersome -- due to the lack of a parameterized constructor, one needs to create an empty (that is, initialized with default values) object and then set the two fields one by one. This PR replaces the default constructor with a parameterized constructor and uses it in the test p2p_compactblocks.py, reducing LOC. No need to pollute the namespace with temporary throw-away message objects anymore.

@fanquake fanquake added the Tests label Aug 23, 2020
@practicalswift
Copy link
Contributor

Concept ACK: DRY and -17 LOC :)

Copy link

@epson121 epson121 left a comment

Choose a reason for hiding this comment

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

Code review ACK 6384419

@guggero
Copy link
Contributor

guggero commented Sep 17, 2020

Code review ACK 6384419.

This creates slightly more new msg_sendcmpct objects instead of reusing the same one. But in this test context that should not matter at all since it's still a small number of objects.

@fanquake fanquake requested a review from maflcko September 19, 2020 06:29
@maflcko maflcko merged commit b99a163 into bitcoin:master Sep 20, 2020
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Sep 21, 2020
…cmpct()

6384419 test: add parameterized constructor for msg_sendcmpct() (Sebastian Falbesoner)

Pull request description:

  While working on the test for bitcoin#19776 I noticed that creating a `sendcmpct` message is quite cumbersome -- due to the lack of a parameterized constructor, one needs to create an empty (that is, initialized with default values) object and then set the two fields one by one. This PR replaces the default constructor with a parameterized constructor and uses it in the test `p2p_compactblocks.py`, reducing LOC. No need to pollute the namespace with temporary throw-away message objects anymore.

ACKs for top commit:
  guggero:
    Code review ACK 6384419.
  epson121:
    Code review ACK 6384419

Tree-SHA512: 3b58d276d714b73abc6cc98d1d52dec5f6026b33f03faaeb7dcbc5d83ac377555179f98b159b2b9ecc8957999c35a1dc082e3c69299c5fde4e35f1bd0587ce9d
@theStack theStack deleted the 20200823-test-extend-msg_sendcmpct_ctor branch December 1, 2020 09:54
Fabcien pushed a commit to Bitcoin-ABC/bitcoin-abc that referenced this pull request Oct 12, 2021
Summary: This is a backport of [[bitcoin/bitcoin#19781 | core#19781]]

Test Plan: `ninja check-functional-extended`

Reviewers: #bitcoin_abc, Fabien

Reviewed By: #bitcoin_abc, Fabien

Subscribers: Fabien

Differential Revision: https://reviews.bitcoinabc.org/D10288
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants