Add compatibility tests for v5.0.x #2 #2396

chatton · 2022-09-26T16:04:24Z

Description

This PR replaces #2370

This is a re-work of the referenced PR. Instead of using a large json file per release branch, this method allows us to define smaller, more maintainable files per release branch / test suite pair.

Sample workflow: here

Update: One additional benefit, each job is limited to creating 256 tasks. A github workflow can have multiple jobs, i.e. transfer, client, incentivized etc. While we are not at that level yet (transfer test is creating 80), splitting up like this leaves more room for growth without refactoring how the tests run in the future.

closes #2370

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

…nal-tests-for-v5-github-matrix

chatton · 2022-09-26T16:05:14Z

.github/workflows/e2e-compatibility-workflow-call.yaml

@@ -0,0 +1,50 @@
+on:
+  workflow_call:


this is a re-usable workflow that we can call for each test suite.

…s://github.com/cosmos/ibc-go into cian/add-additional-tests-for-v5-github-matrix

chatton · 2022-09-26T16:15:06Z

.github/compatibility-test-matrices/release-v5.0.x/incentivized-transfer.json

@@ -0,0 +1,15 @@
+{


these values get expanded so the workflow runs with every permutation of these values.

chatton · 2022-09-27T08:58:38Z

@colin-axner & @charleenfei thank you for reviewing #2370 after some discussion with @crodriguezvega and looking some more into Colin's suggestion, I refactored the workflow to be (much) more maintainable. The workflows are largely the same, it's mostly just the smaller config file we need to manage.

chatton · 2022-09-27T08:59:13Z

.github/workflows/e2e-compatibility.yaml

+    needs:
+      - build-release-image
+      - determine-docker-tag
+    uses: ./.github/workflows/e2e-compatibility-workflow-call.yaml


this is how workflows can be re-used in github actions.

chatton · 2022-09-27T08:59:58Z

.github/workflows/e2e-compatibility.yaml

+    uses: ./.github/workflows/e2e-compatibility-workflow-call.yaml
+    with:
+      docker-tag: "${{ needs.determine-docker-tag.outputs.docker-tag }}"
+      test-suite: "transfer"


these strings map to sub directories, I'm open to alternatives if there are any suggestions!

I'm not sure I understand fully here. Could you provide me with the verbose output? 😅

I initially thought you meant they correspond to the directory under e2e/tests/...

Will we also need to add blocks for interchain-accounts here?

maybe we could rename the value to be something more explicit like matrix-filename WDYT?

I'm easy. I think its fine to leave as is

Whatever is fine by me!

damiannolan

Just left a few questions, I'm getting a better grasp on the workflows, slowly but surely its becoming easier to parse.

The yaml gods have spoken! ⚒️⚡

L:
G:
T:
M:

❤️

damiannolan · 2022-09-27T10:51:18Z

.github/workflows/e2e-compatibility.yaml

+    inputs:
+      release-branch:
+        description: 'Release branch to test'
+        required: true
+        type: choice
+        options:
+          - release/v5.0.x


So do we need to add all release branches we want to test with here?

yes - this is just for a dropdown, if we prefer, we can have a text box and enter anything we like. I just think the dropdown is better UX

damiannolan · 2022-09-27T10:59:27Z

.github/workflows/e2e-compatibility.yaml

+    uses: ./.github/workflows/e2e-compatibility-workflow-call.yaml
+    with:
+      docker-tag: "${{ needs.determine-docker-tag.outputs.docker-tag }}"
+      test-suite: "transfer"


I'm not sure I understand fully here. Could you provide me with the verbose output? 😅

I initially thought you meant they correspond to the directory under e2e/tests/...

damiannolan · 2022-09-27T11:00:08Z

.github/workflows/e2e-compatibility.yaml

+    uses: ./.github/workflows/e2e-compatibility-workflow-call.yaml
+    with:
+      docker-tag: "${{ needs.determine-docker-tag.outputs.docker-tag }}"
+      test-suite: "transfer"


Will we also need to add blocks for interchain-accounts here?

damiannolan · 2022-09-27T11:01:36Z

.github/workflows/e2e-compatibility-workflow-call.yaml

+    needs: load-test-matrix
+    strategy:
+      fail-fast: false
+      matrix: ${{ fromJSON(needs.load-test-matrix.outputs.test-matrix) }}


Aha, so this magic uses load-test-matrix to parse the JSON files and set the values in matrix which is used below to run the e2e test and populate the env?

yes the previous job load-test-matrix stores a json string as its output (the contents of a json file depending on args) and the matrix field accepts dynamic input (or statically defined in the workflow)

This is indeed magic 😅

the process of passing variables between steps is quite cumbersome unfortunately 😓

chatton · 2022-09-27T11:26:57Z

@damiannolan

Will we also need to add blocks for interchain-accounts here?

Currently all the interchain accounts tests are running using the icad, so the state of the release branch in ibc-go should have no impact here right?

I'm not sure I understand fully here. Could you provide me with the verbose output

Right maybe I can add some clarification, the tests themselves exist under e2e/... but the matrix files for the workflow live separately (the new dir added in this PR) compatibility-test-matrices.

This additional field e.g. test-suite: "transfer" is literally just a convention I'm using to pass arguments to the reusable workflow so that we can run any test suite the same way by adhering to a directory structure.

compatibility-test-matrices/<release-<version>/<test-suite>.json

colin-axner

absolutely fabulous! 👑

These e2e test additions are like a dream come true!

colin-axner · 2022-09-28T11:10:03Z

.github/workflows/e2e-compatibility-workflow-call.yaml

+    needs: load-test-matrix
+    strategy:
+      fail-fast: false
+      matrix: ${{ fromJSON(needs.load-test-matrix.outputs.test-matrix) }}


This is indeed magic 😅

colin-axner · 2022-09-28T11:12:22Z

.github/workflows/e2e-compatibility.yaml

+    uses: ./.github/workflows/e2e-compatibility-workflow-call.yaml
+    with:
+      docker-tag: "${{ needs.determine-docker-tag.outputs.docker-tag }}"
+      test-suite: "transfer"


Whatever is fine by me!

colin-axner · 2022-09-28T11:12:47Z

.github/compatibility-test-matrices/release-v5.0.x/client.json

+  "chain-a": ["release-v5.0.x", "v4.1.0", "v3.3.0", "v2.4.0"],
+  "chain-b": ["release-v5.0.x", "v4.1.0", "v3.3.0", "v2.4.0"],


so clean! ❤️

colin-axner · 2022-09-28T11:15:53Z

Currently all the interchain accounts tests are running using the icad, so the state of the release branch in ibc-go should have no impact here right?

I guess the question is if we want to be able to trigger icad compatibility tests from ibc-go as well (long term no, but maybe in the short term?)

chatton · 2022-09-28T12:17:20Z

@colin-axner

I guess the question is if we want to be able to trigger icad compatibility tests from ibc-go as well (long term no, but maybe in the short term?)

Right, we can already do this with the Manual E2E (Icad) test. But I think long term will be able to use simd and it can be added as a new file here.

chatton and others added 22 commits September 22, 2022 14:58

chore: fixing workflow inputs

b5c7185

chore: updating release v5.0.x json

329d8e0

chore: add full set of transfer tests

b86a65a

chore: adding incentivized tests

536e37d

chore: adding tests for client update

d5c758b

chore: change to 3.2 and 2.4

2759086

Merge branch 'main' into cian/add-additional-tests-for-v5

f123c50

corrected tag

1a4cc7d

chore: bump 3.0 to 3.3 and 2.0 to 2.1

c6508d9

Merge branch 'main' into cian/add-additional-tests-for-v5

b52332b

Merge branch 'cian/add-additional-tests-for-v5' into cian/add-additio…

0bfbd52

…nal-tests-for-v5-github-matrix

chore: change release json into non include format

0880657

chore: separate chain a and chain b lists

a18213a

chore: adding workflow call for compatibility tests

65f66e9

chore: corrected needs array

8774891

chore: fixed workflow call

f6fba04

chore: refactor directory structure

0ca1496

chore: added job for client and incentivized transfer

c385f3b

chore: add determine docker tag to needs array

63f4b05

chore: updated versions

3bb23c8

chore: change from chain-a-tag to chain-a

69eeeb8

Merge branch 'main' into cian/add-additional-tests-for-v5-github-matrix

ee16871

chatton commented Sep 26, 2022

View reviewed changes

chatton added 2 commits September 26, 2022 17:09

chore: specify chain image in matrix

1dc3c08

Merge branch 'cian/add-additional-tests-for-v5-github-matrix' of http…

e01789d

…s://github.com/cosmos/ibc-go into cian/add-additional-tests-for-v5-github-matrix

chatton commented Sep 26, 2022

View reviewed changes

Merge branch 'main' into cian/add-additional-tests-for-v5-github-matrix

285a161

chatton marked this pull request as ready for review September 27, 2022 08:57

chatton requested review from colin-axner and AdityaSripal as code owners September 27, 2022 08:57

chatton requested review from crodriguezvega, seantking, charleenfei and damiannolan as code owners September 27, 2022 08:57

chatton commented Sep 27, 2022

View reviewed changes

damiannolan approved these changes Sep 27, 2022

View reviewed changes

Merge branch 'main' into cian/add-additional-tests-for-v5-github-matrix

95c29f7

damiannolan mentioned this pull request Sep 27, 2022

Add compatibility tests for v5.0.x #2370

Closed

9 tasks

colin-axner approved these changes Sep 28, 2022

View reviewed changes

chatton and others added 2 commits September 28, 2022 13:18

Merge branch 'main' into cian/add-additional-tests-for-v5-github-matrix

6d4289f

Merge branch 'main' into cian/add-additional-tests-for-v5-github-matrix

cc81215

chatton merged commit 0d63964 into main Sep 28, 2022

chatton deleted the cian/add-additional-tests-for-v5-github-matrix branch September 28, 2022 12:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add compatibility tests for v5.0.x #2 #2396

Add compatibility tests for v5.0.x #2 #2396

chatton commented Sep 26, 2022 •

edited

Loading

chatton Sep 26, 2022

chatton Sep 26, 2022

chatton commented Sep 27, 2022

chatton Sep 27, 2022

chatton Sep 27, 2022

damiannolan Sep 27, 2022

damiannolan Sep 27, 2022

chatton Sep 27, 2022 •

edited

Loading

damiannolan Sep 27, 2022

colin-axner Sep 28, 2022

damiannolan left a comment

damiannolan Sep 27, 2022

chatton Sep 27, 2022

damiannolan Sep 27, 2022

damiannolan Sep 27, 2022

damiannolan Sep 27, 2022

damiannolan Sep 27, 2022

chatton Sep 27, 2022

colin-axner Sep 28, 2022

chatton Sep 28, 2022

chatton commented Sep 27, 2022

colin-axner left a comment

colin-axner Sep 28, 2022

colin-axner Sep 28, 2022

colin-axner Sep 28, 2022

colin-axner commented Sep 28, 2022

chatton commented Sep 28, 2022

		"chain-a": ["release-v5.0.x", "v4.1.0", "v3.3.0", "v2.4.0"],
		"chain-b": ["release-v5.0.x", "v4.1.0", "v3.3.0", "v2.4.0"],

Add compatibility tests for v5.0.x #2 #2396

Add compatibility tests for v5.0.x #2 #2396

Conversation

chatton commented Sep 26, 2022 • edited Loading

Description

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chatton commented Sep 27, 2022

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

chatton Sep 27, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

damiannolan 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

chatton commented Sep 27, 2022

colin-axner 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

colin-axner commented Sep 28, 2022

chatton commented Sep 28, 2022

chatton commented Sep 26, 2022 •

edited

Loading

chatton Sep 27, 2022 •

edited

Loading