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

Revive gossipsub tests #4286

Closed
wants to merge 15 commits into from
Closed

Revive gossipsub tests #4286

wants to merge 15 commits into from

Conversation

jmcph4
Copy link
Member

@jmcph4 jmcph4 commented May 15, 2023

Issue Addressed

#2335

Proposed Changes

  • Revive two unit tests targeting gossipsub:
    • test_gossipsub_forward
    • test_gossipsub_full_mesh_publish
  • Revive test helper common::build_full_mesh to support the these

Additional Info

N/A

jmcph4 added 15 commits May 9, 2023 10:22
This commit simply uncomments the tests in the relevant module.
This commit replaces `tokio::time::delay_for` calls with the newer
`tokio::time::sleep`.
This commit patches the test code to build a linear network topology
(as the test requires) to use the newer interface for
`common::build_linear`. This commit also hardcodes the fork being used
for this test module to Capella.
This commit cleans up the code for setting up the necessary testing
state by reordering it, adding explanatory inline comments, and
updating the code to use newer interfaces where relevant.
This commit simplifies the inner `match` statement by replacing
`Libp2pEvent` with `NetworkEvent` (meaning we can drop the outermost
layer of nesting) and commenting out the second case (just for now;
it's unclear to me at the moment which `NetworkEvent` variant it should
map to).
This commit promots the existing comment block explaining the test
module to a documentation comment for the entire module.
This commit patches `test_gossipsub_full_mesh_publish` by largely
duplicating the fixes to the other test in the module.
This commit propagates messages to gossipsub via
`Libp2pInstance::publish`.
This commit exposes the libp2p swarm type so that we can make use of it
in the GS tests.
This commit uncomments the `build_full_mesh` test helper so that we can
build meshnets for the GS tests.
This commit uses a meshnet topology for
`test_gossipsub_full_mesh_publish` rather than a linear one.
This commit removes checks for `BehaviourEvent::PeerSubscribed` events
as they are no longer used by the codebase.
This commit rewrites the main loop in `test_gossipsup_forward` to be a
`while let` loop in order to satisfy Clippy.
@jmcph4
Copy link
Member Author

jmcph4 commented May 15, 2023

See #4294.

@jmcph4 jmcph4 closed this May 15, 2023
bors bot pushed a commit that referenced this pull request May 16, 2023
## Issue Addressed

#2335 

## Proposed Changes

 - Remove the `lighthouse-network::tests::gossipsub_tests` module
 - Remove dead code from the `lighthouse-network::tests::common` helper module (`build_full_mesh`)

## Additional Info

After discussion with both @divagant-martian and @AgeManning, these tests seem to have two main issues in that they are:

 - Redundant, in that they don't test anything meaningful (due to our handling of duplicate messages)
 - Out-of-place, in that it doesn't really test Lighthouse-specific functionality (rather libp2p functionality)

As such, this PR supersedes #4286.
ghost pushed a commit to oone-world/lighthouse that referenced this pull request Jul 13, 2023
## Issue Addressed

sigp#2335 

## Proposed Changes

 - Remove the `lighthouse-network::tests::gossipsub_tests` module
 - Remove dead code from the `lighthouse-network::tests::common` helper module (`build_full_mesh`)

## Additional Info

After discussion with both @divagant-martian and @AgeManning, these tests seem to have two main issues in that they are:

 - Redundant, in that they don't test anything meaningful (due to our handling of duplicate messages)
 - Out-of-place, in that it doesn't really test Lighthouse-specific functionality (rather libp2p functionality)

As such, this PR supersedes sigp#4286.
Woodpile37 pushed a commit to Woodpile37/lighthouse that referenced this pull request Jan 6, 2024
## Issue Addressed

sigp#2335 

## Proposed Changes

 - Remove the `lighthouse-network::tests::gossipsub_tests` module
 - Remove dead code from the `lighthouse-network::tests::common` helper module (`build_full_mesh`)

## Additional Info

After discussion with both @divagant-martian and @AgeManning, these tests seem to have two main issues in that they are:

 - Redundant, in that they don't test anything meaningful (due to our handling of duplicate messages)
 - Out-of-place, in that it doesn't really test Lighthouse-specific functionality (rather libp2p functionality)

As such, this PR supersedes sigp#4286.
Woodpile37 pushed a commit to Woodpile37/lighthouse that referenced this pull request Jan 6, 2024
## Issue Addressed

sigp#2335 

## Proposed Changes

 - Remove the `lighthouse-network::tests::gossipsub_tests` module
 - Remove dead code from the `lighthouse-network::tests::common` helper module (`build_full_mesh`)

## Additional Info

After discussion with both @divagant-martian and @AgeManning, these tests seem to have two main issues in that they are:

 - Redundant, in that they don't test anything meaningful (due to our handling of duplicate messages)
 - Out-of-place, in that it doesn't really test Lighthouse-specific functionality (rather libp2p functionality)

As such, this PR supersedes sigp#4286.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant