Skip to content

fix(ci): fix dot/network unit tests #2572

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

Merged
timwu20 merged 3 commits into from
Jun 2, 2022
Merged

fix(ci): fix dot/network unit tests #2572

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
c42efff
move to integration test, hardcode protocol id
timwu20 May 30, 2022
1a8af6c
remove logs, restore original test
timwu20 May 30, 2022
f64f250
cr feedback
timwu20 Jun 2, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .github/workflows/integration-tests.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ jobs:
github.com/ChainSafe/gossamer/dot,
github.com/ChainSafe/gossamer/dot/core,
github.com/ChainSafe/gossamer/dot/rpc/modules,
github.com/ChainSafe/gossamer/lib/babe,
github.com/ChainSafe/gossamer/lib/babe

@qdm12 qdm12 Jun 2, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Is this necessary? 🤔 A bit OOS too. But sure, why not as well 😄

@timwu20 timwu20 Jun 2, 2022

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

😂 didn't mean to do this. I'll remove it

]
runs-on: ubuntu-latest
steps:
Expand Down
6 changes: 4 additions & 2 deletions dot/network/gossip_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,10 +84,12 @@ func TestGossip(t *testing.T) {
Digest: types.NewDigest(),
}

_, err = nodeA.host.send(addrInfoB.ID, "", announceMessage)
time.Sleep(10 * time.Second)

_, err = nodeA.host.send(addrInfoB.ID, "/gossamer/test/0/block-announces/1", announceMessage)
require.NoError(t, err)

time.Sleep(TestMessageTimeout)
time.Sleep(10 * time.Second)

@kishansagathiya kishansagathiya Jun 1, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

locally the test passes even with one second delay. I am guessing the delay requirement might be slightly different. 10s seems little high though.
But seems like only potential way to find an optimum delay would be to run ci bunch of times.

@timwu20 timwu20 Jun 1, 2022 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

These aren't unit tests. I'll create an issue if there already isn't one in the #1472 epic to address this package. A lot of the tests in this package are integration tests and should run in a separate workflow. But we should also write new unit tests for the package if we were to do that. I think running them in parallel with the unit test suite isn't helping when running on github runners.


hash, err := announceMessage.Hash()
require.NoError(t, err)
Expand Down