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.

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

Jump to bottom

Make leadership-schedule test less flaky #4671

Merged
merged 3 commits into from
Jan 10, 2023
Merged

Make leadership-schedule test less flaky #4671

merged 3 commits into from
Jan 10, 2023

Conversation

newhoggy
Copy link
Contributor

@newhoggy newhoggy commented Nov 28, 2022 •
edited
Loading

This PR replaces assertByDeadlineMCustom with byDeadlineM. The latter is better because it catches assertions and retries whereas an assertion in the block of the former will fail the test.

byDeadlineM will also take a poll interval so we don't needlessly poll too often which clutters the output.

The test Babbage.leadership-schedule is re-enabled on MacOS.

The binding maxSlotExpected is moved higher so it is not re-computed unnecessarily.

@newhoggy newhoggy force-pushed the newhoggy/make-leadership-schedule-test-less-flaky branch from 72ec149 to 777a5a1 Compare November 29, 2022 02:59
newhoggy
newhoggy commented Nov 29, 2022
View reviewed changes
cardano-testnet/src/Util/Process.hs
assertByDeadlineMCustom str deadline f
else do
H.annotateShow currentTime
failMessage GHC.callStack $ "Condition not met by deadline: " <> str
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Replace assertByDeadlineMCustom with byDeadlineM. The latter is better because it catches assertions and retries whereas an assertion in the block of the former will fail the test.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

byDeadlineM will also take a poll interval so we don't needlessly poll too often which clutters the output.

@newhoggy newhoggy marked this pull request as ready for review November 29, 2022 03:05
@newhoggy newhoggy force-pushed the newhoggy/make-leadership-schedule-test-less-flaky branch 3 times, most recently from 8346dc5 to 6eff178 Compare November 30, 2022 00:14
newhoggy
newhoggy commented Nov 30, 2022
View reviewed changes
cardano-testnet/test/Main.hs
@@ -28,7 +28,7 @@ tests = pure $ T.testGroup "test/Spec.hs"
-- [ H.ignoreOnMacAndWindows "leadership-schedule" Test.Cli.Alonzo.LeadershipSchedule.hprop_leadershipSchedule
-- ]
, T.testGroup "Babbage"
[ H.ignoreOnMacAndWindows "leadership-schedule" Test.Cli.Babbage.LeadershipSchedule.hprop_leadershipSchedule
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Re-enable leadership schedule test on MacOS.

@newhoggy newhoggy force-pushed the newhoggy/make-leadership-schedule-test-less-flaky branch 2 times, most recently from df58013 to 05b7fbe Compare November 30, 2022 23:39
Jimbo4350
Jimbo4350 previously requested changes Dec 5, 2022
View reviewed changes
cardano-testnet/test/Test/Cli/Alonzo/LeadershipSchedule.hs Show resolved Hide resolved
cardano-testnet/test/Test/Cli/Alonzo/LeadershipSchedule.hs Outdated
H.assert $ not (L.null expectedLeadershipSlotNumbers)

leadershipDeadline <- H.noteShowM $ DTC.addUTCTime 90 <$> H.noteShowIO DTC.getCurrentTime

H.assertByDeadlineMCustom "Leader schedule is correct" leadershipDeadline $ do
-- Leader schedule is correct
H.byDeadlineM 10 leadershipDeadline $ do
Copy link
Contributor

@Jimbo4350 Jimbo4350 Dec 5, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

"Condition not met by deadline" is a somewhat opaque error. Can we implement a custom byDeadlineM to maintain the custom error messages used by assertByDeadlineMCustom?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will require this change: input-output-hk/hedgehog-extras#16

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated to include better assertion messages.

@newhoggy newhoggy force-pushed the newhoggy/make-leadership-schedule-test-less-flaky branch from 05b7fbe to 0ae574f Compare December 31, 2022 03:32
@newhoggy newhoggy dismissed Jimbo4350’s stale review December 31, 2022 03:37

Comments addressed

Jimbo4350
Jimbo4350 reviewed Jan 3, 2023
View reviewed changes
cardano-testnet/test/Test/Cli/Alonzo/LeadershipSchedule.hs Outdated
H.assert $ not (L.null expectedLeadershipSlotNumbers)

leadershipDeadline <- H.noteShowM $ DTC.addUTCTime 90 <$> H.noteShowIO DTC.getCurrentTime

H.assertByDeadlineMCustom "Leader schedule is correct" leadershipDeadline $ do
-- Leader schedule is correct
H.byDeadlineM 10 leadershipDeadline "Wait for a leadership at least as new as the highest one we expect" $ do
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we bother with this check if we already assert the following: H.assert $ L.length (expectedLeadershipSlotNumbers \\ leaderSlots) <= 1

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We have to wait until a time when all the slots we expect should exist. The assertion doesn't do the wait. Without the wait we would check too early and the slots won't be there.

Copy link
Contributor

@Jimbo4350 Jimbo4350 Jan 5, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

"Wait for a leadership at least as new as the highest one we expect" is opaque to me. I think here some verbosity is fine because it will make failure diagnosis easier, especially to those unfamiliar with the code. What about "Wait for chain to surpass all expected leadership slots". And we can add a comment as follows: "We need enough time to pass such that the expected leadership slots generated by the leadership-schedule command have actually occurred."

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated!

cardano-testnet/test/Test/Cli/Babbage/LeadershipSchedule.hs Outdated Show resolved Hide resolved
@newhoggy newhoggy force-pushed the newhoggy/make-leadership-schedule-test-less-flaky branch from 0ae574f to bd2d757 Compare January 4, 2023 22:24
@newhoggy newhoggy force-pushed the newhoggy/make-leadership-schedule-test-less-flaky branch from 3141ca1 to 84de454 Compare January 5, 2023 13:45
@newhoggy newhoggy force-pushed the newhoggy/make-leadership-schedule-test-less-flaky branch from 84de454 to 4281970 Compare January 10, 2023 21:20
@newhoggy newhoggy merged commit 834b4ad into master Jan 10, 2023
@iohk-bors iohk-bors bot deleted the newhoggy/make-leadership-schedule-test-less-flaky branch January 10, 2023 22:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Jimbo4350 Jimbo4350 Jimbo4350 approved these changes

@dcoutts dcoutts Awaiting requested review from dcoutts

@erikd erikd Awaiting requested review from erikd

@JaredCorduan JaredCorduan Awaiting requested review from JaredCorduan

@CarlosLopezDeLara CarlosLopezDeLara Awaiting requested review from CarlosLopezDeLara

@MarcFontaine MarcFontaine Awaiting requested review from MarcFontaine

@LudvikGalois LudvikGalois Awaiting requested review from LudvikGalois

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@newhoggy @LudvikGalois @Jimbo4350