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

test: reduce number of happyPath steps #1179

Closed
wants to merge 4 commits into from

Conversation

MSalopek
Copy link
Contributor

@MSalopek MSalopek commented Aug 2, 2023

Description

Closes: #1178

Attempt to reduce the number of steps in e2e happy path to reduce CI runtimes.


Author Checklist

All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.

I have...

  • Included the correct type prefix in the PR title
  • Targeted the correct branch (see PR Targeting)
  • Provided a link to the relevant issue or specification
  • Reviewed "Files changed" and left comments if necessary
  • Confirmed all CI checks have passed

Reviewers Checklist

All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.

I have...

  • Confirmed the correct type prefix in the PR title
  • Confirmed all author checklist items have been addressed
  • Confirmed that this PR does not change production code

@MSalopek MSalopek requested a review from a team as a code owner August 2, 2023 08:59
@MSalopek MSalopek self-assigned this Aug 2, 2023
@MSalopek MSalopek marked this pull request as draft August 2, 2023 09:00
stepsDoubleSignOnProviderAndConsumer("consu"), // carol double signs on provider, bob double signs on consumer
stepsSubmitEquivocationProposal("consu", 2), // now prop to tombstone bob is submitted and accepted
stepsStartRelayer(),
stepsConsumerRemovalPropNotPassing("consu", 3), // submit removal prop but vote no on it - chain should stay
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Checked in shortHappyPathSteps

Copy link
Contributor

Choose a reason for hiding this comment

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

When is the short happy path executed?
My understanding is that the normal happy path is executed during CI (e.g., automated tests before we merge), but not clear about the short happy_ path.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's a misnomer. short uses cometmock but it is not running in CI yet

@@ -22,13 +22,9 @@ var happyPathSteps = concatSteps(
stepsRedelegateForOptOut("consu"),
stepsDowntimeWithOptOut("consu"),
stepsRedelegate("consu"),
stepsDowntime("consu"),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

No need to test for downtime twice, testing for downtime with soft opt-out also covers "normal" downtime.

Copy link
Contributor

@insumity insumity Aug 2, 2023

Choose a reason for hiding this comment

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

Although both use the downtimeSlashAction action, in stepsDowntimeWithOptOut there doesn't seem that slashing takes place since alice has < 5% of the total voting power. While in stepsDowntime, bob is indeed slashed.
Additionally stepsDowntime is also performing an unjailValidatorAction.
Therefore, wouldn't we weaken the happyPathSteps by removing the stepsDowntime?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It will, but there's an additional issue that proposes changes to how this runs in the CI:

We can have multiple jobs running in parallel and make the execution time shorter

@MSalopek MSalopek marked this pull request as ready for review August 2, 2023 10:55
@@ -21,7 +21,7 @@ func stepsRejectEquivocationProposal(consumerName string, propNumber uint) []Ste
ValPowers: &map[validatorID]uint{
validatorID("alice"): 509,
validatorID("bob"): 500,
validatorID("carol"): 495,
validatorID("carol"): 501,
Copy link
Contributor

@insumity insumity Aug 2, 2023

Choose a reason for hiding this comment

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

Why were the voting powers changed here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Some steps that changed the voting power were removed. Steps are executed sequentially, so if you remove one of them you have to account for the removal in other places.

Copy link
Contributor

@insumity insumity left a comment

Choose a reason for hiding this comment

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

Left some comments but otherwise, LGTM.

@MSalopek MSalopek force-pushed the masa/1178-happy-path-cleanup branch from 1c9771f to 5457e71 Compare August 2, 2023 17:30
@MSalopek
Copy link
Contributor Author

MSalopek commented Aug 2, 2023

@insumity We can add cometmock tests very shortly after this. I'm debugging some issues at the moment.

EDIT:
Here is a draft PR with cometmock, but it fails at the last step, and sometimes at step 17/18.
#1181

@shaspitz
Copy link
Contributor

shaspitz commented Aug 7, 2023

Changes seem safe given the info 👍, are we cool removing e2e testing of downtime jailing in CI as @insumity mentioned? Seems @MSalopek has plans to readd this to CI shortly after? As it is a core part of the protocol that we should regression test often

@MSalopek
Copy link
Contributor Author

MSalopek commented Aug 9, 2023

Setting this back to draft. A better approach was chosen. We can do this later, the current approach is to add cometmock first
#1181

@MSalopek MSalopek marked this pull request as draft August 9, 2023 09:37
@MSalopek MSalopek closed this Aug 9, 2023
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.

reduce number of steps in e2e happyPath
3 participants