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

Refactor p2p unit tests #2475

Merged
merged 13 commits into from
Dec 12, 2023
Merged

Refactor p2p unit tests #2475

merged 13 commits into from
Dec 12, 2023

Conversation

joshua-kim
Copy link
Contributor

@joshua-kim joshua-kim commented Dec 12, 2023

Why this should be merged

Raise the bar on our p2p unit tests. This prevents us from having to spin up goroutines in the unit tests so the test logic just goes straight down. This also adds some interfaces so that we can decouple the common.Sender from the p2p layer, since it becomes difficult to test p2p as the sender struct is shared between many things.

How this works

Rewrites pretty much all of the UTs

How this was tested

UTs

@joshua-kim joshua-kim force-pushed the sender-refactor branch 3 times, most recently from 6d1a013 to 2c8c805 Compare December 12, 2023 08:52
Signed-off-by: Joshua Kim <[email protected]>
@joshua-kim joshua-kim marked this pull request as ready for review December 12, 2023 08:58
network/p2p/sender.go Outdated Show resolved Hide resolved
network/p2p/sender.go Outdated Show resolved Hide resolved
network/p2p/network_test.go Show resolved Hide resolved
network/p2p/network_test.go Show resolved Hide resolved
network/p2p/network_test.go Show resolved Hide resolved
network/p2p/sender.go Outdated Show resolved Hide resolved
Copy link
Contributor

@abi87 abi87 left a comment

Choose a reason for hiding this comment

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

lgtm pending the set.Of change.

Signed-off-by: Joshua Kim <[email protected]>
Signed-off-by: Joshua Kim <[email protected]>
Signed-off-by: Joshua Kim <[email protected]>
Signed-off-by: Joshua Kim <[email protected]>
@joshua-kim joshua-kim requested a review from danlaine December 12, 2023 18:55
Signed-off-by: Joshua Kim <[email protected]>
@StephenButtolph StephenButtolph added this to the v1.10.18 milestone Dec 12, 2023
@StephenButtolph StephenButtolph added the networking This involves networking label Dec 12, 2023
Signed-off-by: Joshua Kim <[email protected]>
Signed-off-by: Joshua Kim <[email protected]>
Signed-off-by: Joshua Kim <[email protected]>
Copy link
Contributor

@StephenButtolph StephenButtolph left a comment

Choose a reason for hiding this comment

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

I'm not really sure that the FakeSender is needed to remove the creation of the goroutines (We've used the callback version to update variables in the past + in some of these tests already).

I definitely think this is cleaner than what was there before though - so lgtm

@StephenButtolph StephenButtolph added this pull request to the merge queue Dec 12, 2023
Merged via the queue into dev with commit ac5a00e Dec 12, 2023
16 checks passed
@StephenButtolph StephenButtolph deleted the sender-refactor branch December 12, 2023 23:05
joshua-kim added a commit that referenced this pull request Dec 13, 2023
commit 82fbc97
Author: Stephen Buttolph <[email protected]>
Date:   Tue Dec 12 18:30:09 2023 -0500

    Add ACP signaling (#2476)

commit ac5a00e
Author: Joshua Kim <[email protected]>
Date:   Tue Dec 12 17:42:32 2023 -0500

    Refactor p2p unit tests (#2475)

    Signed-off-by: Joshua Kim <[email protected]>
    Co-authored-by: Dan Laine <[email protected]>

commit 0b2b109
Author: Dhruba Basu <[email protected]>
Date:   Tue Dec 12 16:48:28 2023 -0500

    `vms/platformvm`: Verify txs before building a block (#2359)

    Co-authored-by: Stephen Buttolph <[email protected]>

commit 4be744e
Author: Joshua Kim <[email protected]>
Date:   Tue Dec 12 15:08:48 2023 -0500

    P2P AppError handling (#2248)

    Signed-off-by: Joshua Kim <[email protected]>
    Co-authored-by: Stephen Buttolph <[email protected]>

commit 7963115
Author: Dhruba Basu <[email protected]>
Date:   Tue Dec 12 14:37:59 2023 -0500

    `vms/platformvm`: Add `TestBuildBlockForceAdvanceTime` test (#2472)

    Co-authored-by: Stephen Buttolph <[email protected]>

commit dc472ec
Author: Dhruba Basu <[email protected]>
Date:   Tue Dec 12 14:37:43 2023 -0500

    `vms/platformvm`: Permit usage of the `Transactions` field in `BanffProposalBlock` (#2451)

    Co-authored-by: Stephen Buttolph <[email protected]>

Signed-off-by: Joshua Kim <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
networking This involves networking
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants