chore: Add integration for equivocation with CometMock #1192
Conversation
p-offtermatt
changed the title
p-offtermatt
changed the title
| @@ -36,7 +36,7 @@ test-e2e-short: | |||
| # run only happy path E2E tests with cometmock | |||
| # this set of traces does not test equivocation but it does check downtime | |||
| test-e2e-short-cometmock: | |||
| go run ./tests/e2e/... --short-happy-path --use-cometmock --use-gorelayer | |||
| go run ./tests/e2e/... --cometmock-happy-path --use-cometmock --use-gorelayer | |||
There was a problem hiding this comment.
I think all of this is great and very exciting.
However a small suggestion, maybe we refactor the main.go flag parsing so that --cometmock-happy-path uses go relayer by default so the flag does not need to be specified.
The flag could just be -cometmock-happy-path --use-cometmock.
This can also be done as part of a larger refactor, I'm just asking for opinions
There was a problem hiding this comment.
The reason why it is not like this is that the gorelayer can be used without cometmock, too.
(just not CometMock without the Gorelayer)
I suggest leaving it as it, in the anticipation that one day CometMock can be used with hermes, too (and it does not seem too bad until then to have it as-is)
Lmk if this sounds convincing to you
There was a problem hiding this comment.
To me it seems implicit that if you're specifying --cometmock-happy-path, both --use-cometmock and --use-gorelayer would default to true
There was a problem hiding this comment.
pls note that PR #1191 is changing the way tests can be selected.
Keeping selection of tests and infra seperated regarding parameters is better from a usablity perspective (full flexibility with reduced set of option).
I'd be in favor in not renaming the 'test case' and rather not having 'cometmock' in the name as it leads to assumptions that infra/test setup is using cometmock which is not the case. Running --short-happy-path --use-cometmock (with or without implicit usage of --use-gorelayer) is clearer IMO.
There was a problem hiding this comment.
assumptions that infra/test setup is using cometmock which is not the case. Running --short-happy-path --use-cometmock (with or without implicit usage of --use-gorelayer) is clearer IMO.
I think this is a good approach to take.
| } else { // tr.useCometMock | ||
| validatorAddress := tr.GetValidatorAddress(action.chain, action.validator) | ||
|
|
||
| method := "cause_double_sign" |
| @@ -36,7 +36,7 @@ test-e2e-short: | |||
| # run only happy path E2E tests with cometmock | |||
| # this set of traces does not test equivocation but it does check downtime | |||
| test-e2e-short-cometmock: | |||
| go run ./tests/e2e/... --short-happy-path --use-cometmock --use-gorelayer | |||
| go run ./tests/e2e/... --cometmock-happy-path --use-cometmock --use-gorelayer | |||
There was a problem hiding this comment.
To me it seems implicit that if you're specifying --cometmock-happy-path, both --use-cometmock and --use-gorelayer would default to true
| } else { // tr.useCometMock | ||
| validatorAddress := tr.GetValidatorAddress(action.chain, action.validator) | ||
|
|
||
| method := "cause_double_sign" |
There was a problem hiding this comment.
Curious where the script lives that actually handles "cause_double_sign"?
There was a problem hiding this comment.
it's a cometmock RPC method, it does not live here. It's a feature of cometmock
Description
Closes: #1190
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...
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...