test: Add trace input via JSON #1265

p-offtermatt · 2023-09-06T08:39:49Z

Description

Closes: #1240

Changes are as follows:

Main e2e test entry file: For the test main.go, change the behavior flags are processed:
- --tc now has the form --tc teststeps::testrunner
- new flag --test-file filename::testrunner
- These changes are made so that the steps and the runner are separated. This is more future proof, and for test-file, specifying the test runner separately is necessary.
Trace format:
- The PR introduces a trace format that is just a JSON serialization of a step slice. This means it makes use of all the actions we already have.
- tests/e2e/tracehandler_testdata/ just contains the serializations of a few existing steps. These can be regenerated by this test:
  
  interchain-security/tests/e2e/trace_handlers_test.go
  
  Line 84 in e2175c2
  
  func TestWriteExamples(t *testing.T) {
- The serialization has two complicated aspects: Actions and proposals
  - JSON does not add types, so marshalling/unmarshalling interfaces is hard because they can contain different types
  - See http://gregtrowbridge.com/golang-json-serialization-with-interfaces/
  - I solve this by, on marshal/unmarshal, adding/reading an extra type string that contains the type of the action/proposal. unfortunately, these have to be maintained when we add new actions, but I don't see any easier way.
  - This is the reason for these two massive switch-cases:
    - interchain-security/tests/e2e/json_utils.go
      
      Line 50 in e2175c2
      
      case "main.submitConsumerAdditionProposalAction":
    - interchain-security/tests/e2e/json_utils.go
      
      Line 403 in e2175c2
      
      case "main.TextProposal":
- To do the serialization, I also needed to make all fields of actions/state public, essentially everything that needs to go into the JSON. see this https://stackoverflow.com/questions/11126793/json-and-dealing-with-unexported-fields
  - This is the vast majority of changes e.g. action becomes Action, chainId becomes ChainId, ...
- I am using Rapid to do some fuzz-testing for the parser/writer, just checking that all kinds of []Steps can be marshalled/unmarshalled without problems. See https://github.com/cosmos/interchain-security/blob/e2175c206ac1cb8a641fa914b179414d609a51db/tests/e2e/action_rapid_test.go#L486
  - This also needs to be maintained - when we add new actions, we should probably also add them here. however, it's not horrible when this is not updated - it just means some things are not tested by Rapid, so nothing breaks immediately. Currently, this should include all the actions that exist in the e2e tests.

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

* Add other steps to read/write test * Improve error logging in trace handler test * Push ActionType handling into Unmarshal/Marshal and out of own class * Rename generic trace handler files to be clear they are for json * Rename to marshalling to be more generic * Add marshal/unmarshal for proposals * Export unexported field * Add test for marshal/unmarshalling chain state * Fix pointer issues * Fix typo: action -> proposal * Log proposal string in test * Use json.RawMessage instead of map[string]any * For uniformity, also use RawMessage for step unmarshalling * Add tests for extra proposal types * Add more proposal types to test and unify names * Add handling for ParamsProposal * Regenerate traces * Chore: Export forgotten field * Use string, not int, to help marshal/unmarshal

* Add other steps to read/write test * Improve error logging in trace handler test * Push ActionType handling into Unmarshal/Marshal and out of own class * Rename generic trace handler files to be clear they are for json * Rename to marshalling to be more generic * Add marshal/unmarshal for proposals * Export unexported field * Add test for marshal/unmarshalling chain state * Fix pointer issues * Fix typo: action -> proposal * Log proposal string in test * Use json.RawMessage instead of map[string]any * For uniformity, also use RawMessage for step unmarshalling * Add tests for extra proposal types * Add more proposal types to test and unify names * Add handling for ParamsProposal * Regenerate traces * Chore: Export forgotten field * Use string, not int, to help marshal/unmarshal * Add rapid to go.mod and .sum * Add rapidpbt for chainState marshalling * Rename file to make clear it only relates to chainState * Add error return to TraceWriter and TraceParser * gst * Add generators for actions and steps, utilize in test driver * Restrict range for time * Add test for time marshal/unmarshal * Make time have a lower bound, since negative numbers are not supported by RFC3339 * Improve label string for argument to time.Unix * Correct lower bound for time: 1900 years negative instead of 2000 * Convert timestamp to utc * Format file * Ignore testdata folder * Add go comment * Add docstring to GetTraceGen

p-offtermatt · 2023-09-15T09:33:27Z

tests/e2e/main.go

 )

+var selectedTestfiles TestSet
+
+var stepChoices = map[string]StepChoice{


I separated step choices from test runs.

Why: When we input files, we need to give the test runs separately anyways. This makes that behaviour universal. Also seems more future proof, e.g. for running the same steps with different test runs/contexts

To a semi outsider like myself, the abstractions here are not super intuitive.

"steps" seems like a set of steps to make in running a test.

"test runs" seems synonymous to "steps"

p-offtermatt · 2023-09-15T09:34:33Z

tests/e2e/steps.go

@@ -66,15 +66,15 @@ var slashThrottleSteps = concatSteps(
 	stepsStopChain("consu", 2),
 )

-var democracySteps = concatSteps(
+var democracyRewardsSteps = concatSteps(


The naming was confusing before: democracySteps were the input for democracy-reward, and rewardDenomConsumerSteps were the input for democracy. This makes the mapping of test cases to step names more consistent.

p-offtermatt · 2023-09-15T09:35:34Z

.github/workflows/automated-tests.yml

@@ -56,3 +56,17 @@ jobs:
          go-version: "1.20"
      - name: E2E tests
        run: make test-e2e-short-cometmock
+  Trace-Tests:


Add a test that reads traces to the CI to check that trace-reading does not break. Could be expanded in the future

p-offtermatt · 2023-09-15T09:36:23Z

tests/e2e/.gitignore

@@ -0,0 +1 @@
+testdata


Ignore temporary folder that rapid puts test failures in

shaspitz · 2023-09-18T18:37:49Z

I'm about to review, first impression of reading the description: this work all seems awesome. There's a lot of changes added in one PR tho. Is it possible to split out these individual components?

CLI changes: adding flags, changing parameter format, etc.
Pure refactors like making fields public
Meat of changes, JSON as test trace input

Totally understand if it's not worth the effort to split everything out at this point, just throwing out the idea

shaspitz

Really cool stuff! My comments are mostly in regard to e2e test abstraction rather than functionality directly related to the PR title. Might be worth splitting things out if not too challenging

shaspitz · 2023-09-18T18:45:03Z

.github/workflows/manual-e2e.yml

@@ -36,7 +36,7 @@ jobs:
        with:
          go-version: "1.20" # The Go version to download (if necessary) and use.
      - name: E2E changeover test
-        run: go run ./tests/e2e/... --tc changeover
+        run: go run ./tests/e2e/... --tc changeover::changeover


From description

for test-file, specifying the test runner separately is necessary

It seems to me like specifying testrunner is mostly relevant to the --test-file flag. Is it neccessary to also require that callers specify the testrunner when using --tc?

Asking because the cli for e2e tests is becoming quite verbose to run a simple test from the manual (golang defined) test cases

Also we should document the required format for each flag in a readme if that's not already done

Also we should document the required format for each flag in a readme if that's not already done

You can see it by running --help, do you think that is good enough?

From description

for test-file, specifying the test runner separately is necessary

It seems to me like specifying testrunner is mostly relevant to the --test-file flag. Is it neccessary to also require that callers specify the testrunner when using --tc?

Asking because the cli for e2e tests is becoming quite verbose to run a simple test from the manual (golang defined) test cases

Okay, I can see that. I'm between making it optional and removing it for the test cases, and I think for now I will remove it for --tc. Can be added back later if we need it.

shaspitz · 2023-09-18T18:48:51Z

tests/e2e/action_rapid_test.go

@@ -0,0 +1,485 @@
+package main
+
+import (


Am I understanding correctly that this file serves to fuzz test the ser/deser of json specified e2e actions?

shaspitz · 2023-09-18T18:53:04Z

tests/e2e/action_rapid_test.go

+
+// This needs to be adjusted manually when new actions are added and should
+// include generators for all actions that are mentioned in main.go/runStep.
+func GetActionGen() *rapid.Generator[any] {


Will prob need more context to properly review this file

Currently splitting this out, then we can e.g. go over that part, or I'll write more explanation

tests/e2e/actions.go

shaspitz · 2023-09-18T19:24:41Z

tests/e2e/json_marshal_test.go

+	"github.com/davecgh/go-spew/spew"
+)
+
+func TestProposalUnmarshal(t *testing.T) {


Is there a reason these tests don't use rapid like action_rapid_test.go does?

Yes. I wanted to expliticly give a json string as an input, since this is hard to do with Rapid - we can get a json input by marshalling the rapid-generated objects, but this implies trusting that the marshal function does things correctly. this is able to test unmarshal and marshal in a slightly more isolated manner

shaspitz · 2023-09-18T19:29:15Z

tests/e2e/json_utils.go

+	case "main.submitConsumerAdditionProposalAction":
+		var a submitConsumerAdditionProposalAction
+		err := json.Unmarshal(rawAction, &a)
+		if err != nil {


Is it possible to have

if err != nil { return nil, err }

live outside the switch statement so as to not repeat code. Maybe just create a var err error above the switch that can be set

Good call, f5b0355

shaspitz · 2023-09-18T19:29:51Z

tests/e2e/json_utils.go

+	case "main.ConsumerAdditionProposal":
+		prop := ConsumerAdditionProposal{}
+		err := json.Unmarshal(inputMap, &prop)
+		if err != nil {


Same as above with local err variable

shaspitz · 2023-09-18T19:35:01Z

tests/e2e/main.go

 )

+var selectedTestfiles TestSet
+
+var stepChoices = map[string]StepChoice{


To a semi outsider like myself, the abstractions here are not super intuitive.

"steps" seems like a set of steps to make in running a test.

"test runs" seems synonymous to "steps"

shaspitz · 2023-09-18T19:37:08Z

tests/e2e/main.go

-func getTestCases(selection TestSet) (tests []testRunWithSteps) {
+type testRunWithSteps struct {
+	testRun TestRun
+	steps   []Step


Related to above, it seems like "steps" should be a field of TestRun, as in a test run is a high level abstraction with config + actual steps to be executed

I have renamed TestRun to TestConfig, since that is much more fitting to reality.

Good call, easy to get blind to these weird, historical naming choices. I've refactored this.

The abstractions are now:
A TestRun is made of
steps []Step
testConfig TestConfig

lmk if you still find it confusing

shaspitz · 2023-09-18T19:40:06Z

tests/e2e/main.go

+	testRuns []string
+}
+
+type TestRunChoice struct {


TestRun already has a name field right? Is this new type neccessary?

Co-authored-by: Shawn <44221603+smarshall-spitzbart@users.noreply.github.com>

p-offtermatt · 2023-09-19T07:08:35Z

Thanks for the review, very useful stuff! I'll give splitting this as much as possible a try and apply your comments.

p-offtermatt · 2023-09-19T07:38:36Z

Closing in favor of a feature branch https://github.com/cosmos/interchain-security/tree/feat/trace-format

Fix merge

039fa81

github-actions bot added the C:Testing Assigned automatically by the PR labeler label Sep 6, 2023

p-offtermatt and others added 28 commits September 6, 2023 13:31

Lint file

bc18a8c

Clean filepath before usage

de89a18

Wait downtime_duration, not single block

a4347f7

Revert formatting change

65fdd40

Readd comment string

21a6f55

Revert formatting change

b7217c8

Add back comment string

55aa940

Revert formatting change

ef41ba8

Regenerate test data, better action registry

47a8970

Add tests and handle proposals

199ca5c

Fix merge

9b07d2c

Fix ibc-go version

b204f92

Fix custom marshal: no pointer receiver

5b82e96

Add test for proposal unwrapping and fix bugs in it

c8fd833

Merge branch 'main' into ph/875-revived-json-trace-format

0114a81

Fix tests

0f6537c

Attempt to fix json parsing trouble

91c97aa

Add todos to make pickup easier

89182b7

Change registries to switch statements

4bab3dd

Update dependency versions

d88deec

Add rapid tests for actions

81a504e

Add comment to action marshalling

0a165d2

Don't capitalize function args

3a6d9f1

Fix marshal tests

a81f92d

Merge branch 'main' into ph/875-revived-json-trace-format

d24f732

Regenerate traces

ddea97c

p-offtermatt added 4 commits September 13, 2023 17:27

Start rewriting logic for choosing test cases

4dcc9e0

Finish map for test runner choice

54c7f61

Fix usage infos and negate contains

fbec9d8

Fix Makefiles with new test case format

799f866

github-actions bot added C:Build Assigned automatically by the PR labeler C:CI Assigned automatically by the PR labeler labels Sep 15, 2023

p-offtermatt added 4 commits September 15, 2023 10:30

Rename steps to be more explicit

efd6d53

Change steps/test run delimiter

60d510f

Add support for reading traces from files

c09ddee

Add trace test to automated tests

53deeff

p-offtermatt commented Sep 15, 2023

View reviewed changes

Fix typo: frAction

04e319f

p-offtermatt commented Sep 15, 2023

View reviewed changes

tests/e2e/.gitignore

@@ -0,0 +1 @@

testdata

Copy link

Contributor Author

p-offtermatt Sep 15, 2023

Choose a reason for hiding this comment

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

Ignore temporary folder that rapid puts test failures in

p-offtermatt and others added 2 commits September 15, 2023 11:40

Remove mistakenly introduced arg from CometMock

b00b22a

Remove trace folder

e2175c2

p-offtermatt marked this pull request as ready for review September 15, 2023 09:59

p-offtermatt requested a review from a team as a code owner September 15, 2023 09:59

p-offtermatt mentioned this pull request Sep 15, 2023

chore: Implement JSON trace format (#878) #1243

Closed

shaspitz reviewed Sep 18, 2023

View reviewed changes

Update tests/e2e/actions.go

d271175

Co-authored-by: Shawn <44221603+smarshall-spitzbart@users.noreply.github.com>

p-offtermatt closed this Sep 19, 2023

This was referenced Sep 19, 2023

tests: Export struct fields in the e2e tests #1313

Merged

test: Add json marshal/unmarshal for test traces #1314

Merged

Add flag for reading test traces and add them in the CI #1315

Merged

test: introduce flag for trace input to e2e framework #1318

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test: Add trace input via JSON #1265

test: Add trace input via JSON #1265

p-offtermatt commented Sep 6, 2023 •

edited

Loading

p-offtermatt Sep 15, 2023 •

edited

Loading

shaspitz Sep 18, 2023

p-offtermatt Sep 15, 2023

p-offtermatt Sep 15, 2023

p-offtermatt Sep 15, 2023

shaspitz commented Sep 18, 2023

shaspitz left a comment

shaspitz Sep 18, 2023

shaspitz Sep 18, 2023

p-offtermatt Sep 20, 2023

p-offtermatt Sep 20, 2023

shaspitz Sep 18, 2023

p-offtermatt Sep 19, 2023

shaspitz Sep 18, 2023

p-offtermatt Sep 19, 2023

shaspitz Sep 18, 2023

p-offtermatt Sep 19, 2023

shaspitz Sep 18, 2023

p-offtermatt Sep 19, 2023 •

edited

Loading

shaspitz Sep 18, 2023

p-offtermatt Sep 19, 2023

shaspitz Sep 18, 2023

shaspitz Sep 18, 2023

p-offtermatt Sep 20, 2023

shaspitz Sep 18, 2023

p-offtermatt commented Sep 19, 2023 •

edited

Loading

p-offtermatt commented Sep 19, 2023

test: Add trace input via JSON #1265

test: Add trace input via JSON #1265

Conversation

p-offtermatt commented Sep 6, 2023 • edited Loading

Description

Author Checklist

Reviewers Checklist

p-offtermatt Sep 15, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shaspitz commented Sep 18, 2023

shaspitz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

p-offtermatt Sep 19, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

p-offtermatt commented Sep 19, 2023 • edited Loading

p-offtermatt commented Sep 19, 2023

p-offtermatt commented Sep 6, 2023 •

edited

Loading

p-offtermatt Sep 15, 2023 •

edited

Loading

p-offtermatt Sep 19, 2023 •

edited

Loading

p-offtermatt commented Sep 19, 2023 •

edited

Loading