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

Dynamically generate compatibility test files #7547

Open
wants to merge 48 commits into
base: main
Choose a base branch
from

Conversation

chatton
Copy link
Contributor

@chatton chatton commented Nov 11, 2024

Description

closes: #4872

This PR does the following

  • Adds a python script which determines which tests should be run based off of annotations in the test files.
  • Adds the relevant annotations to all the test files (to mirror what the static config files were doing)
  • Removes all the static config files.
  • Removes the "unreleased" workflow. This wasn't really used much, if the functionality this tool provided is desired again in the future, we can just update the python script to handle this use case.
  • Fixes the manual build simd image workflow (there was a missing white space 😭 )
  • Removes the script that updated the compat files with manual inputs.

Here is a manual trigger I ran last night https://github.com/cosmos/ibc-go/actions/runs/11806430988/job/32912856842

I've since addressed the failing tests, but won't run again until tonight to not interfere with everyone else since it spins up a huge number of runners. The issues fixed were

  • some matricies were spinning up more than 256 jobs, I added support to split these up into chain-a/chain-b format
  • some tests were running against versions they should not have been running into, I updated the annotations in the tests to filter out versions we don't want to run.

I think we can still merge this PR if people are happy with the general approach, and then I can do a follow up to fix any issues that still persist after running the tests again tonight.

I dedicate this PR to @crodriguezvega who dealt with these compatibility files for far too long! 🫡


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 the 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 and Go style guide.
  • Wrote unit and integration tests.
  • Updated relevant documentation (docs/).
  • Added relevant godoc comments.
  • Provide a conventional commit message to follow the repository standards.
  • Include a descriptive changelog entry when appropriate. This may be left to the discretion of the PR reviewers. (e.g. chores should be omitted from changelog)
  • Re-reviewed Files changed in the GitHub PR explorer.
  • Review SonarCloud Report in the comment section below once CI passes.

@@ -111,6 +112,7 @@ func (s *InterchainAccountsParamsTestSuite) TestControllerEnabledParam() {
})
}

// compatibility:TestHostEnabledParam:from_versions: v9.0.0,v8.4.0,v7.5.0
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this syntax is arbitrary, we can change this to whatever we like. I figured it is a straightforward enough way of specifying custom data about specific tests.

Comment on lines +269 to +270
if "(t *testing.T)" in line:
return re.search(r"func\s+(.*)\(", line).group(1)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

a little bit gross, we can try and come up with a cleaner way of doing this if people feel strongly about it.

Copy link
Contributor

@bznein bznein left a comment

Choose a reason for hiding this comment

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

In general LGTM but I need to take another look :) Left some fly-by comments

version = version[1:]
if version.startswith("release-"):
# strip off the release prefix and parse the actual version
return parse_version(version[len("release-"):])
Copy link
Contributor

Choose a reason for hiding this comment

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

[nit] for consistency, we can either do above return parse_version(version[1:]) or here version = version[len("release-"):]

if version.startswith("v"):
# semver versions do not include a "v" prefix.
version = version[1:]
if version.startswith("release-"):
Copy link
Contributor

Choose a reason for hiding this comment

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

[nit] maybe extract "release" to const?

Comment on lines +142 to +169
entry = (release_version, version, test, entrypoint, relayer, chain_image)

if entry not in seen and chain in ("chain-a", "all"):
include_entries.append(
{
"chain-a": release_version,
"chain-b": version,
"entrypoint": entrypoint,
"test": test,
"relayer-type": relayer,
"chain-image": chain_image
}
)
seen.add(entry)

entry = (version, release_version, test, entrypoint, relayer, chain_image)
if entry not in seen and chain in ("chain-b", "all"):
include_entries.append(
{
"chain-a": version,
"chain-b": release_version,
"entrypoint": entrypoint,
"test": test,
"relayer-type": relayer,
"chain-image": chain_image
}
)
seen.add(entry)
Copy link
Contributor

Choose a reason for hiding this comment

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

[q] could we refactor this so that there is less duplication?

raise ValueError(f"key {k} not found in {item.keys()}")

if len(compatibility_json["include"]) > 256:
# if this error occurs, split out the workflow into two jobs, one for chain-a and one for chain-b
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe we can add this info to the error itself so that we don't have to go look at the code?

Copy link

sonarcloud bot commented Nov 13, 2024

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.

Automate generation of compatibility json files
2 participants