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

chore(dot/parachain/overseer): fix TestHandleBlockEvents the flaky test #3707

Merged

Conversation

axaysagathiya
Copy link
Contributor

@axaysagathiya axaysagathiya commented Jan 18, 2024

Changes

overseer

  • If we close overseerToSub1, subsystem2 could still send a message for subsystem1 via subsystemToOverseer, and it will be panic.
  • we can close overseerToSubsystem channels only if the subsystemToOverseer channel is closed.

IMO, we should not close either of them.
That is why I removed the code to close overseerToSubsystem channels.

Used a waitGroup in the test to wait till we received all the messages in mocked subsystems before we called the stop function of overseer.

Tests

go test -tags integration github.com/ChainSafe/gossamer

Issues

closes #3717

Primary Reviewer

@kishansagathiya @EclesioMeloJunior

@kishansagathiya kishansagathiya marked this pull request as draft January 18, 2024 09:12
@axaysagathiya axaysagathiya marked this pull request as ready for review January 22, 2024 10:34
@P1sar P1sar added C-simple Minor changes changes, no additional research needed. Good first issue/review. S-subsystems-overseer issues related to Polkadot host overseer functionality. S-tests issue related to adding new tests. labels Jan 22, 2024
Copy link
Contributor

@kishansagathiya kishansagathiya left a comment

Choose a reason for hiding this comment

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

Currently TestHandleBlockEvents fails a lot of times in CI. This PR shows a bigger problem of how to gracefully shutdown overseer and subsystems.

Even though, this solution is not ideal, since it keeps the channels open, it is necessary to unblock other PRs. Currently, we are having to run CI some 6-7 to pass it due to this test.
Hence approving this. Will tackle the problem to shutting down overseer and subsystems separately.

@axaysagathiya axaysagathiya merged commit b131ff9 into feat/parachain Jan 23, 2024
21 of 22 checks passed
@axaysagathiya axaysagathiya deleted the axay/chore/overseer/test-handle-block-events branch January 23, 2024 13:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-simple Minor changes changes, no additional research needed. Good first issue/review. S-subsystems-overseer issues related to Polkadot host overseer functionality. S-tests issue related to adding new tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants