Skip to content
This repository has been archived by the owner on Feb 1, 2023. It is now read-only.

Fix flaky DontHaveTimeoutManger tests #495

Merged
merged 6 commits into from
Jun 24, 2021

Conversation

hannahhoward
Copy link
Contributor

Goals

Fix flaky tests for the DontHaveTimeoutManger

Implementation

Replace clock with a mock, so that we are able to mock out time in tests, modify tests to use mock time and be more predictable.

Also avoid AfterFunc cause of potential problems with channel consumption.

@hannahhoward
Copy link
Contributor Author

Part of ipfs/boxo#86

} else {
dhtm.checkForTimeoutsTimer.Stop()
dhtm.checkForTimeoutsTimer.Reset(until)
}
}

func (dhtm *dontHaveTimeoutMgr) consumeTimeouts() {
for {
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this should be a for loop, because checkForTimeouts() is a recursive call

Copy link
Contributor Author

Choose a reason for hiding this comment

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

note this will only get called once cause then checkForTimeoutsTimer will not be null.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think that the for loop will continue looping forever though - so I'm suggesting that we only have the select (without putting it inside a for loop)

checkForTimeoutsTimer *time.Timer
checkForTimeoutsTimer *clock.Timer
// used for testing -- signal when a scheduled timeout check has happened
signal chan struct{}
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: suggest renaming signal to something like timeoutChecked

@hannahhoward hannahhoward force-pushed the fix/fix-dont-have-manager-timeouts branch from ee7f583 to f60e47c Compare June 5, 2021 00:59
Base automatically changed from web3-bot/sync to master June 24, 2021 17:20
count message received before callback so that the count is always accurate at the time of counting
The TestSessionWithPeers test was most commonly failing cause of a don't
have timeout, which triggered simulated don't have message for all CIDs
on the peer with content, which triggered a re-broadcast, causing peers
with no content to receive additional wants
s/SetSendDontHavesOnTimeout/SetSimulateDontHavesOnTimeout
convery DontHaveTimeoutMgr to use clock interface, use mocks in tests to make tests predictable and
fast
@Stebalien Stebalien force-pushed the fix/fix-dont-have-manager-timeouts branch from f60e47c to 38aae7e Compare June 24, 2021 17:24
@Stebalien Stebalien merged commit bfac454 into master Jun 24, 2021
@Stebalien
Copy link
Member

These tests appear to be passing now.

@Stebalien Stebalien deleted the fix/fix-dont-have-manager-timeouts branch June 24, 2021 17:31
@aschmahmann aschmahmann mentioned this pull request Aug 23, 2021
62 tasks
Jorropo pushed a commit to Jorropo/go-libipfs that referenced this pull request Jan 26, 2023
…nager-timeouts

Fix flaky DontHaveTimeoutManger tests

This commit was moved from ipfs/go-bitswap@bfac454
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants