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

tests: Export struct fields in the e2e tests #1313

Merged
merged 4 commits into from
Sep 19, 2023

Conversation

p-offtermatt
Copy link
Contributor

@p-offtermatt p-offtermatt commented Sep 19, 2023

Description

Closes: Part of #1265

This PR makes the fields of the relevant structs used in traces exported.
This is necessary preparation for the json format, since JSON marshal/unmarshal has problems with unexported fields, see https://stackoverflow.com/questions/11126793/json-and-dealing-with-unexported-fields.

To review this, please check that the only changes this PR makes are setting the fields of structs and types used in the e2e tests to public. In particular, the fields of all actions need to be exported.
There are also two small refactors that I have marked with comments.


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

@p-offtermatt p-offtermatt requested a review from a team as a code owner September 19, 2023 07:38
@github-actions github-actions bot added the C:Testing Assigned automatically by the PR labeler label Sep 19, 2023
@@ -66,15 +66,15 @@ var slashThrottleSteps = concatSteps(
stepsStopChain("consu", 2),
)

var democracySteps = concatSteps(
var democracyRewardsSteps = concatSteps(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Small refactor: 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.

@@ -65,8 +65,8 @@ since causing light client attacks is not implemented.`,
},
"happy-path": {testRun: DefaultTestRun(), steps: happyPathSteps, description: "happy path tests"},
"changeover": {testRun: ChangeoverTestRun(), steps: changeoverSteps, description: "changeover tests"},
"democracy-reward": {testRun: DemocracyTestRun(true), steps: democracySteps, description: "democracy tests allowing rewards"},
"democracy": {testRun: DemocracyTestRun(false), steps: rewardDenomConsumerSteps, description: "democracy tests"},
"democracy-reward": {testRun: DemocracyTestRun(true), steps: democracyRewardsSteps, description: "democracy tests allowing rewards"},
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Small refactor: The step names were mismatched with the test-case names before

Copy link
Contributor

Choose a reason for hiding this comment

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

Nice catch!

@p-offtermatt p-offtermatt linked an issue Sep 19, 2023 that may be closed by this pull request
`--node`, tr.getValidatorNode(action.chain, action.from),
`--title`, action.Title,
`--description`, action.Description,
`--type`, action.PropType,
Copy link
Contributor

Choose a reason for hiding this comment

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

this --type seems new, is this expected?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, exactly the kind of thing I was looking for. Missed that, fixed in d1edc8d

ChainA: ChainID("provi"),
ChainB: ChainID(consumer1),
Port: "provider",
Channel: 0, // consumer1 channel
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: you made this change before in other files.
s/channel/Channel

Copy link
Contributor Author

Choose a reason for hiding this comment

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

changed it back to small channel in comments in other places, thanks!

ChainA: ChainID("provi"),
ChainB: ChainID(consumer2),
Port: "provider",
Channel: 1, // consumer2 channel
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: similarly to previous comment

Copy link
Contributor Author

Choose a reason for hiding this comment

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

same as previous comment

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.

LGTM. Thanks for the "To review this [...] exported." text in your PR.

@p-offtermatt
Copy link
Contributor Author

Thanks for the review, I will merge this when the automated tests have passed.

@shaspitz
Copy link
Contributor

This PR should be merged directly to main imo, as it will pollute the diff to any future feature branch PR.

ie. we don't benefit from split PRs if this PR is merged into a feature branch

Copy link
Contributor

@shaspitz shaspitz left a comment

Choose a reason for hiding this comment

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

Looks great!

@@ -65,8 +65,8 @@ since causing light client attacks is not implemented.`,
},
"happy-path": {testRun: DefaultTestRun(), steps: happyPathSteps, description: "happy path tests"},
"changeover": {testRun: ChangeoverTestRun(), steps: changeoverSteps, description: "changeover tests"},
"democracy-reward": {testRun: DemocracyTestRun(true), steps: democracySteps, description: "democracy tests allowing rewards"},
"democracy": {testRun: DemocracyTestRun(false), steps: rewardDenomConsumerSteps, description: "democracy tests"},
"democracy-reward": {testRun: DemocracyTestRun(true), steps: democracyRewardsSteps, description: "democracy tests allowing rewards"},
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice catch!

@p-offtermatt p-offtermatt changed the base branch from feat/trace-format to main September 19, 2023 18:37
@p-offtermatt p-offtermatt merged commit 99a171f into main Sep 19, 2023
13 checks passed
@p-offtermatt p-offtermatt deleted the ph/refactor-e2e-structs branch September 19, 2023 18:39
shaspitz added a commit that referenced this pull request Sep 20, 2023
* build(deps): bump actions/checkout from 3 to 4 (#1257)

Bumps [actions/checkout](https://github.com/actions/checkout) from 3 to 4.
- [Release notes](https://github.com/actions/checkout/releases)
- [Changelog](https://github.com/actions/checkout/blob/main/CHANGELOG.md)
- [Commits](actions/checkout@v3...v4)

---
updated-dependencies:
- dependency-name: actions/checkout
  dependency-type: direct:production
  update-type: version-update:semver-major
...

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* build(deps)!: bump github.com/cosmos/ibc-go/v7 from 7.2.0 to 7.3.0 (#1258)

* build(deps): bump github.com/cosmos/ibc-go/v7 from 7.2.0 to 7.3.0

Bumps [github.com/cosmos/ibc-go/v7](https://github.com/cosmos/ibc-go) from 7.2.0 to 7.3.0.
- [Release notes](https://github.com/cosmos/ibc-go/releases)
- [Changelog](https://github.com/cosmos/ibc-go/blob/main/CHANGELOG.md)
- [Commits](cosmos/ibc-go@v7.2.0...v7.3.0)

---
updated-dependencies:
- dependency-name: github.com/cosmos/ibc-go/v7
  dependency-type: direct:production
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <[email protected]>

* add changelog entries

---------

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: mpoke <[email protected]>

* build(deps): bump github.com/cosmos/cosmos-sdk from 0.47.4 to 0.47.5 (#1259)

* build(deps): bump github.com/cosmos/cosmos-sdk from 0.47.3 to 0.47.5

Bumps [github.com/cosmos/cosmos-sdk](https://github.com/cosmos/cosmos-sdk) from 0.47.3 to 0.47.5.
- [Release notes](https://github.com/cosmos/cosmos-sdk/releases)
- [Changelog](https://github.com/cosmos/cosmos-sdk/blob/main/CHANGELOG.md)
- [Commits](cosmos/cosmos-sdk@v0.47.3...v0.47.5)

---
updated-dependencies:
- dependency-name: github.com/cosmos/cosmos-sdk
  dependency-type: direct:production
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <[email protected]>

* add changelog entries

---------

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: mpoke <[email protected]>

* chore: Separate semver (#1217)

separate semver

* docs: cleanup changelog (#1260)

fix changelog

* fix!: validate MsgTransfer before calling Transfer() (#1244)

* validate MsgTransfer

* add TestSendRewardsToProvider

* update DefaultConsumerUnbondingPeriod to 14 days

* update changelog

* fix linter

* fix test

* apply review suggestions

* update changelog

* docs: Create adr-012-separate-releasing.md (#1229)

* Create adr-011-separate-releasing.md

* Update docs/docs/adrs/adr-011-separate-releasing.md

Co-authored-by: Philip Offtermatt <[email protected]>

* adr 12 not 11

* correct that we use postfix not prefix

* explanation on example release flow

---------

Co-authored-by: Philip Offtermatt <[email protected]>

* fix: remove addr validation for provider fee pool addr param (#1262)

* fix: remove validation for provider chain address since we cannot validate it properly on consumer

* add changelog entry

* docs: update CHANGELOG.md for `v3.2.0-consumer` release (#1268)

* Update CHANGELOG.md

* Update CHANGELOG.md

* chore: remove legacy_ibc_testing (rebased on main) (#1185)

* chore: remove legacy_ibc_testing

* fix import

* introduce provider.UnmarshalConsumerPacketData

* use patched ibc-go

* fix annoying import order

* test: ignore key ordering

Seems like ibctesting.GenerateKeys can returns keys in different orders
when test cover is enabled.

* fix after rebase

* replace allinbits/ibc-go with patched cosmos/ibc-go

Also fix panic in TestRedelegationNoConsumer

* fix lint

* use released ibc-go 7.3.0

* add newPacketFromConsumer/Provider helper funcs

* constructPacket method return ccv type instead of []byte

* tests: increase timeout in nightly e2e for multiconsumer (#1272)

* test: Add light client attack to e2e tests (#1249)

* Add light client attack to e2e tests

* Add details about the attack types

* Rename validatorAddress to validatorPrivateKeyAddress

* Add URL to CometMock to flag

* Improve wording for light client attack run

* Add submitting equivocation proposals and change consu->consumerName

* build(deps): bump google.golang.org/grpc from 1.57.0 to 1.58.0 (#1284)

Bumps [google.golang.org/grpc](https://github.com/grpc/grpc-go) from 1.57.0 to 1.58.0.
- [Release notes](https://github.com/grpc/grpc-go/releases)
- [Commits](grpc/grpc-go@v1.57.0...v1.58.0)

---
updated-dependencies:
- dependency-name: google.golang.org/grpc
  dependency-type: direct:production
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* ci: update mergify and dependabot for v2.x-lsm (#1281)

update mergify and dependabot for v2.x-lsm

* build(deps): bump github.com/oxyno-zeta/gomock-extra-matcher from 1.1.0 to 1.2.0 (#1286)

build(deps): bump github.com/oxyno-zeta/gomock-extra-matcher

Bumps [github.com/oxyno-zeta/gomock-extra-matcher](https://github.com/oxyno-zeta/gomock-extra-matcher) from 1.1.0 to 1.2.0.
- [Release notes](https://github.com/oxyno-zeta/gomock-extra-matcher/releases)
- [Changelog](https://github.com/oxyno-zeta/gomock-extra-matcher/blob/master/release.config.js)
- [Commits](oxyno-zeta/gomock-extra-matcher@v1.1.0...v1.2.0)

---
updated-dependencies:
- dependency-name: github.com/oxyno-zeta/gomock-extra-matcher
  dependency-type: direct:production
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* feat!: provider proposal for changing reward denoms (#1280)

* new provider prop type

* add methods and tests for new prop, update docs

* remove old tx, fix tests

* e2e handling

* fix command type

* boilerplate

* fix e2e tests

* Update CHANGELOG.md

* lint

* validate denoms

* Update proposal.go

* rm msg string

* fix tests

* rm chain in change denom action

* lint

* test for invalid denom

* events for both add and remove

* Update proposal_test.go

* docs: introduce ADR on slashing on the provider chain (#1252)

* initial draft of the adr

* small change on intro.md to avoid huge diff

* clean up

* take into account Marius' comments

* rewrite v0.47 Cosmos SDK link because it was returning 404 and redirecting

* more cleaning up

* update based on comments

* removed confusing sentence on voting power

* add missing ADRs in the intro file

* add tokens to power conversion

* add paragraph on key pruning and references to light-client attacks code

* Update template title

* Took into account comments.

* augment pseudocode to skip redelegation/undelegation entries

* fix identation issue

---------

Co-authored-by: Karolos Antoniadis <[email protected]>

* ci: update mergify and dependabot for v3.2.x-consumer (#1297)

* update ymls

* add newline

---------

Co-authored-by: mpoke <[email protected]>

* ci: update bots for v2.1.x-provider-lsm (#1305)

update bots for v2.1.x-provider-lsm

* Fix `build` in makefile (#1303)

fix build in makefile

* refactor: Vaguely named consumer structs (#1288)

* Rename GenesisState proto
* make proto-gen
* proto rename: Params -> ConsumerParams
* make proto-gen
* App, fix type change for Param renaming
* App, fix type change for GenesisState renaming
* Fix unit-tests for GenesisState renaming
* Fix unit-tests for Params renaming
* Addressed review comments
* Fix linter issues

* build(deps): bump google.golang.org/grpc from 1.58.0 to 1.58.1 (#1306)

Bumps [google.golang.org/grpc](https://github.com/grpc/grpc-go) from 1.58.0 to 1.58.1.
- [Release notes](https://github.com/grpc/grpc-go/releases)
- [Commits](grpc/grpc-go@v1.58.0...v1.58.1)

---
updated-dependencies:
- dependency-name: google.golang.org/grpc
  dependency-type: direct:production
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* feat: increment consensus ver and register migration (#1295)

* increment consensus ver and register migration

* Update CHANGELOG.md

* Update migration.go

* Update module.go

* tests: Export struct fields in the e2e tests (#1313)

* Make struct fields in e2e exported

* Un-capitalize binary name

* Remove prop type

* Channel to channel in comments

---------

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: mpoke <[email protected]>
Co-authored-by: Philip Offtermatt <[email protected]>
Co-authored-by: Dmitry Kolupaev <[email protected]>
Co-authored-by: Thomas Bruyelle <[email protected]>
Co-authored-by: MSalopek <[email protected]>
Co-authored-by: insumity <[email protected]>
Co-authored-by: Karolos Antoniadis <[email protected]>
Co-authored-by: Simon Noetzlin <[email protected]>
Co-authored-by: bernd-m <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C:Testing Assigned automatically by the PR labeler
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Modify the e2e framework to take traces as an input
3 participants