Skip to content
This repository has been archived by the owner on Dec 4, 2024. It is now read-only.

Refactor the network tests #314

Merged
merged 33 commits into from
Jan 5, 2022
Merged

Refactor the network tests #314

merged 33 commits into from
Jan 5, 2022

Conversation

zivkovicmilos
Copy link
Contributor

@zivkovicmilos zivkovicmilos commented Dec 24, 2021

Description

This PR refactors the tests in the networking package, fixes bugs and adds new coverage tests.

Additionally, this PR resolves all pending linting errors (3937fbc) and modifies the GH Actions workflow file for the linter (886c4d5)

Encountered issues:

  • Servers were using taken ports in concurrent environments in tests
  • Invalid event waits within tests
  • Low timeouts set for test completion, causing tests to fail even though they succeeded

Failed linting

Currently, the linting checks fail on GH actions because some structures are unused due to test skips.
This PR should wait for #312, which contains the libp2p package updates before unskipping the tests and fixing linting errors that arise from this.

Changes include

  • Bugfix (non-breaking change that solves an issue)
  • New feature (non-breaking change that adds functionality)
  • Breaking change (change that is not backwards-compatible and/or changes current functionality)

Checklist

  • I have assigned this PR to myself
  • I have added at least 1 reviewer
  • I have tested this code
  • I have updated the README and other relevant documents (guides...)
  • I have added sufficient documentation both in code, as well as in the READMEs

Additional comments

Due to a known and fixed libp2p bug (libp2p/go-libp2p#1090), some test have been skipped for now:

  • TestSimpleGossip
  • TestSyncer_PeerDisconnected

The Polygon SDK is running version v0.12.0 of libp2p, where this issue is still present.

These tests will be un-skipped as soon as #312 is merged (after the libp2p package is updated to v0.14.0

EDIT: Tests un-skipped ✅

@zivkovicmilos zivkovicmilos self-assigned this Dec 24, 2021
@zivkovicmilos zivkovicmilos marked this pull request as ready for review December 25, 2021 22:17
@zivkovicmilos zivkovicmilos marked this pull request as draft December 25, 2021 23:07
@zivkovicmilos zivkovicmilos marked this pull request as ready for review December 26, 2021 13:18
Copy link
Contributor

@brkomir brkomir left a comment

Choose a reason for hiding this comment

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

Overall looks ok! Fix the linter errors and we are good to go

consensus/ibft/snapshot_test.go Show resolved Hide resolved
e2e/transaction_test.go Show resolved Hide resolved
helper/keccak/keccak.go Outdated Show resolved Hide resolved
helper/keccak/pool.go Outdated Show resolved Hide resolved
jsonrpc/dispatcher.go Outdated Show resolved Hide resolved
network/discovery_test.go Outdated Show resolved Hide resolved
network/testing.go Outdated Show resolved Hide resolved
network/testing.go Outdated Show resolved Hide resolved
Copy link
Contributor

@Kourin1996 Kourin1996 left a comment

Choose a reason for hiding this comment

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

Looks good!

Copy link
Contributor

@lazartravica lazartravica left a comment

Choose a reason for hiding this comment

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

Flawless, great job.

Good thinking about the helper methods for bootstrapping the network.

@zivkovicmilos zivkovicmilos merged commit ad1b15e into develop Jan 5, 2022
@zivkovicmilos zivkovicmilos deleted the fix/network-testing branch January 7, 2022 11:16
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.

4 participants