test: add missing comparison of node1's mempool in MempoolPackagesTest #29948
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code CoverageFor detailed information about the code coverage, see the test coverage report. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. ConflictsReviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first. |
There was a problem hiding this comment.
Tested ACK e912717. The code looks good to me and the test execution is successful.
|
ACK e912717 looks good to me |
|
Hello everyone, |
|
@Umiiii I am not sure why you are asking me because I am not familiar with this change yet. I'm sure a maintainer will take a look at this change soon. You need to have a bit of patience though, there are a lot of open PRs and Issues and this being open for 3 days is not a very long time. And in general, more review is always better, 3 ACKs is the rule of thumb for simple but non-trivial changes. And please remove the @ mentions in your commit message. The commit message is merged with the change and that leads to a lot of unnecessary notifications for those that are tagged. |
There was a problem hiding this comment.
tACK e912717
Make successful, all functional tests pass. Left couple suggestions.
Also for some reason checking out this PR using this method doesn't work, I had to checkout this particular commit instead. Is it because the forked repo's branch is also called master?
https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/reviewing-changes-in-pull-requests/checking-out-pull-requests-locally
|
|
||
| # TODO: test ancestor size limits | ||
| entry0 = self.nodes[0].getmempoolentry(tx) | ||
| entry1 = self.nodes[1].getmempoolentry(tx) |
There was a problem hiding this comment.
With this, isn't the check on line 201 redundant now?
There was a problem hiding this comment.
mempool1 only stores string(txid), while getmempoolentry contains more information that we need to check, so no, it is not redundant. Also see: https://developer.bitcoin.org/reference/rpc/getmempoolentry.html
There was a problem hiding this comment.
Hmm so not redundant keeping in mind that testing getrawmempool is also needed, got it.
| @@ -251,10 +251,14 @@ def run_test(self): | |||
| assert tx in mempool1 | |||
| for tx in chain[CUSTOM_DESCENDANT_LIMIT:]: | |||
| assert tx not in mempool1 | |||
| # TODO: more detailed check of node1's mempool (fees etc.) | |||
|
|
|||
| # TODO: test descendant size limits | |||
| entry0 = self.nodes[0].getmempoolentry(tx) | ||
| entry1 = self.nodes[1].getmempoolentry(tx) | ||
| assert not entry0['unbroadcast'] | ||
| assert not entry1['unbroadcast'] | ||
| assert_equal(entry1['fees']['base'], entry0['fees']['base']) | ||
| assert_equal(entry1['vsize'], entry0['vsize']) | ||
| assert_equal(entry1['depends'], entry0['depends']) |
There was a problem hiding this comment.
Nit: This portion is copy-paste of the above block, can be extracted in a function compareMempoolsEntry(entry0, entry1)
There was a problem hiding this comment.
That's on purpose, just to align with the existing coding style within this file (and other testing code). Those unit tests don't actually introduce new functions while comparing two entries (and the entries' variables).
|
ACK e912717 |
#29941 Recreated a pull request because there was a conflict. Trying to resolve the conflict but the old one automatically closed.
Add missing comparison for TODO comments in
mempool_packages.pyAlso, notice that the ancestor size limits and descendant size limits actually implemented in #21800 , so I removed the todo for those two size limits.