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

Add compatibility tests for v5.0.x #2370

Closed
wants to merge 10 commits into from

Conversation

chatton
Copy link
Contributor

@chatton chatton commented Sep 22, 2022

Description

While the json file is a bit noisy and has a lot of duplication, this is something we could spend time to generate with tooling if it becomes an issue. For now I think it will be fine to maintain by hand.

This workflow was triggered by the UI.

After bumping versions for client test

bump v2 and v3

You can test this by going to

Actions -> Compatibility E2E -> run workflow (choose this branch - not main) -> click Run Workflow


Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.

  • Targeted PR against correct branch (see CONTRIBUTING.md)
  • Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
  • Code follows the module structure standards.
  • Wrote unit and integration tests
  • Updated relevant documentation (docs/) or specification (x/<module>/spec/)
  • Added relevant godoc comments.
  • Added a relevant changelog entry to the Unreleased section in CHANGELOG.md
  • Re-reviewed Files changed in the Github PR explorer
  • Review Codecov Report in the comment section below once CI passes

type: choice
options:
- release/v5.0.x
inputs:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

missed the inputs field which enables the UI drop down.

Comment on lines 468 to 482
{
"chain-binary": "simd",
"chain-a-tag": "v3.0.0",
"chain-b-tag": "release-v5.0.x",
"chain-image": "ghcr.io/cosmos/ibc-go-simd",
"entrypoint": "TestClientTestSuite",
"test": "TestClientUpdateProposal_Succeeds"
},
{
"chain-binary": "simd",
"chain-a-tag": "v2.0.0",
"chain-b-tag": "release-v5.0.x",
"chain-image": "ghcr.io/cosmos/ibc-go-simd",
"entrypoint": "TestClientTestSuite",
"test": "TestClientUpdateProposal_Succeeds"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

these 2 tests are failing, 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.

sync'd with @charleenfei and it looks like the TrustingPeriod field was backported to 2.4 and 3.2 (not 2.0/3.0!)

{
"chain-binary": "simd",
"chain-a-tag": "release-v5.0.x",
"chain-b-tag": "v3.0.0",
Copy link
Contributor

@charleenfei charleenfei Sep 23, 2022

Choose a reason for hiding this comment

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

i think we could use 3.3.x here (the security release)

},
{
"chain-binary": "simd",
"chain-a-tag": "v3.0.0",
Copy link
Contributor

Choose a reason for hiding this comment

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

same comment as above

{
"chain-binary": "simd",
"chain-a-tag": "release-v5.0.x",
"chain-b-tag": "v2.0.0",
Copy link
Contributor

Choose a reason for hiding this comment

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

2.1.0 is the security release

Copy link
Contributor

@charleenfei charleenfei left a comment

Choose a reason for hiding this comment

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

just the comment on using the security releases for the v3 and v2 lines otherwise nice!

Copy link
Contributor

@colin-axner colin-axner left a comment

Choose a reason for hiding this comment

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

Excellent! ❤️

@@ -1,12 +1,485 @@
{
"include": [
{
"chain-binary": "simd",
"chain-a-tag": "release-v5.0.x",
Copy link
Contributor

Choose a reason for hiding this comment

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

Food for thought: Base tests which should work for any version combination, maybe it'd be possible to swap out the "branch-to-test" and then only maintain files for tests which require logic for which cross versions to 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.

Actually I was just speaking with @crodriguezvega about this and I think we may be able to make some changes and leverage some of the built in matrix generation features of github actions to make some of these permutations re-usable (swap out the branch as you said).

I will update the versions as you suggested and take a look into seeing if this can be simplified to work a bit better with our use case!

Copy link
Contributor

@colin-axner colin-axner left a comment

Choose a reason for hiding this comment

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

We should probably change v4.0.0 -> v4.1.0 and v2.1.0 -> v2.4.0

It might be nice to actually add every minor release since there can be state machine breaking changes for each minor release, but this looks good to me for now

{
"chain-binary": "simd",
"chain-a-tag": "release-v5.0.x",
"chain-b-tag": "v4.0.0",
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"chain-b-tag": "v4.0.0",
"chain-b-tag": "v4.1.0",

},
{
"chain-binary": "simd",
"chain-a-tag": "v2.1.0",
Copy link
Contributor

@colin-axner colin-axner Sep 26, 2022

Choose a reason for hiding this comment

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

Suggested change
"chain-a-tag": "v2.1.0",
"chain-a-tag": "v2.4.0",

I think this is the latest release. Should we just use the release branch?

Edit: would prefer to use final release instead of release branch so as not to test against unreleased changes

@damiannolan
Copy link
Member

shall we close this one in favour of #2396

@chatton chatton closed this Sep 27, 2022
@chatton chatton deleted the cian/add-additional-tests-for-v5 branch September 27, 2022 12:24
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.

4 participants