Skip to content

test: expand LimitOrphan and EraseForPeer coverage #30082

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Aug 5, 2024
Merged

test: expand LimitOrphan and EraseForPeer coverage #30082

merged 2 commits into from
Aug 5, 2024

Conversation

instagibbs
Copy link
Member

Inspired by refactorings in #30000 as the coverage appeared a bit sparse.

Added some minimal border value testing, timeouts, and tightened existing assertions.

@DrahtBot
Copy link
Contributor

DrahtBot commented May 10, 2024 •
edited
Loading

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type Reviewers
ACK rkrux, glozow, achow101

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #28031 (Package Relay 1/3: Introduce TxDownloadManager and improve orphan-handling by glozow)

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.

@DrahtBot DrahtBot added the Tests label May 10, 2024
@instagibbs instagibbs mentioned this pull request May 10, 2024
Merged
glozow
glozow reviewed May 13, 2024
View reviewed changes
src/test/orphanage_tests.cpp Outdated
Comment on lines 207 to 224
// Jump ORPHAN_TX_EXPIRE_TIME minutes, orphan should be timed out on limiting
SetMockTime(GetTime() + 20 * 60);
BOOST_CHECK_EQUAL(orphanage.CountOrphans(), 1);
orphanage.LimitOrphans(1, rng);
BOOST_CHECK_EQUAL(orphanage.CountOrphans(), 0);
Copy link
Member

Choose a reason for hiding this comment

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

Maybe out of scope but I wonder if it would be best to add a current_time parameter to the TxOrphanage methods to make it easier to test and fuzz the expiration (also should have a test for the sorting for GetChildrenFromSamePeer results)

Copy link
Member Author

Choose a reason for hiding this comment

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

probably a good idea, though you might have better opinions on orphanage interface. I tightened the time testing a bit more by setting mocktime at beginning, then checking boundary conditions via subsequent setting of the time.

@instagibbs instagibbs force-pushed the 2024-05-orphan_limit_eraseforpeer branch from bb2389a to dd7bf60 Compare May 13, 2024 16:41
glozow
glozow reviewed May 17, 2024
View reviewed changes
Copy link
Member

@glozow glozow left a comment

Choose a reason for hiding this comment

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

lgtm

@instagibbs instagibbs force-pushed the 2024-05-orphan_limit_eraseforpeer branch from dd7bf60 to 6f70bf4 Compare June 7, 2024 14:03
@instagibbs instagibbs force-pushed the 2024-05-orphan_limit_eraseforpeer branch from 6f70bf4 to 6831f39 Compare June 7, 2024 14:11
@instagibbs
Copy link
Member Author

rebased due to silent merge conflict, with cleanups suggested by @glozow 👍

glozow
glozow reviewed Jun 7, 2024
View reviewed changes
Copy link
Member

@glozow glozow left a comment

Choose a reason for hiding this comment

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

almost ack 6831f39, thanks for taking suggestions

@instagibbs instagibbs force-pushed the 2024-05-orphan_limit_eraseforpeer branch from 6831f39 to eaf4de0 Compare June 7, 2024 15:06
glozow
glozow reviewed Jun 10, 2024
View reviewed changes
Copy link
Member

@glozow glozow left a comment

Choose a reason for hiding this comment

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

ACK eaf4de0

rkrux
rkrux approved these changes Jun 17, 2024
View reviewed changes
Copy link
Contributor

@rkrux rkrux left a comment

Choose a reason for hiding this comment

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

tACK eaf4de0

Make successful, so are all the unit tests and functional test. Asked couple questions for my understanding.

@instagibbs instagibbs force-pushed the 2024-05-orphan_limit_eraseforpeer branch from eaf4de0 to 172c1ad Compare June 17, 2024 13:57
rkrux
rkrux approved these changes Jun 17, 2024
View reviewed changes
Copy link
Contributor

@rkrux rkrux left a comment

Choose a reason for hiding this comment

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

reACK 172c1ad

Thanks for very quickly addressing my comments @instagibbs, appreciate it!

@DrahtBot DrahtBot requested a review from glozow June 17, 2024 18:26
glozow
glozow reviewed Jul 18, 2024
View reviewed changes
Copy link
Member

@glozow glozow left a comment

Choose a reason for hiding this comment

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

reACK 172c1ad

@instagibbs
Copy link
Member Author

@achow101 RFM?

@DrahtBot DrahtBot mentioned this pull request Aug 1, 2024
Closed
@achow101
Copy link
Member

achow101 commented Aug 5, 2024

ACK 172c1ad

@achow101 achow101 merged commit dd7e12a into bitcoin:master Aug 5, 2024
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@glozow glozow glozow left review comments

@rkrux rkrux rkrux approved these changes

Assignees
No one assigned
Labels
Tests
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@instagibbs @DrahtBot @achow101 @rkrux @glozow