rework Github test workflow when splitting core into submodules

[robert-zaremba]

Summary

Ref: #10325 (comment)

After breaking down the current SDK module setup into submodules, the split-test-files and test jobs in test.yaml github workflow doesn't make sense.

Problem Definition

• We can only run a go test for packages in the same module.
• We fixed the existing test workflow in fix: github test workflow #10325, however we will need to redo the core module test workflow to parallelize module testing.

Proposal

Rework the workflow to accomodate proper submodule testing

• parallel execution
• properly ignore unnecessary tests
• make sure dependencies are handled correctly (eg if module A depends on module B, module B changed but module A didn't change, we should still run module A tests).

---

For Admin Use

• Not duplicate issue
• Appropriate labels applied
• Appropriate contributors tagged
• Contributor assigned/self-assigned

[aaronc]

I think we really need module specific CI - the checks that are needed for a given module should only run when that module is changed and then CI should generally be pretty quick. To make this work we probably want to avoid go.mod replace directives when expressing intermodule dependencies and instead always reference some tag.

One thing I've noticed in GitHub for required checks is that I think we can use generic names like build, test, etc. and have different actions that satisfy those checks. Not sure if that is clear but basically I think we need to define the required test phases but they can be implemented with slightly different actions per module.

[aaronc]

So currently these are the status checks we require for master in the protected branches settings:

Let's compare this with what happens in actual checks:

Github has some strange rules around how the jobs in github actions are converted to status checks. For example in test.yml, this generates status checks named build, build (arm64), etc. and tests (00), tests (01), etc. ignoring the name of the action (Tests / Code Coverage).

This has the unpleasant consequence that if we have jobs conditional on what has changed, then some checks won't get triggered and then the PR will be blocked. And it's also too complicated sometimes to manually check off the required status checks correctly when CI is so involved and sometimes changes. Currently, a lot of checks which I think should be required aren't required I think for these reasons (like build (arm64)).

So I recommend we find a way to:

1. have uniform job names for github actions that will always be triggered no matter which code changed (maybe using job.name ?), and
2. have actions that are conditionally triggered based on what code changes

Probably we can get away with 3 required status checks: lint, build and test if we can get this working. Then we would need to write actions which run the minimal checks for the submodules which changed in those PRs, but those actions always need to trigger lint, build and test checks.

[robert-zaremba]

I think we really need module specific CI - the checks that are needed for a given module should only run when that module is changed and then CI should generally be pretty quick

In #10325 I've added a script which checks if a module should be tested or not. However simple diff check is not enough. We have false negatives, when a module is not in a diff, but it's dependency is in a diff.

We should update that script and add dependency checks.

[robert-zaremba]

For lint , build ... we can do something similar - write a python / bash script . The job will still be test, but details will be handled by the script. And if there is nothing to test / verify, the script will return 0 (success).

[aaronc]

In #10325 I've added a script which checks if a module should be tested or not. However simple diff check is not enough. We have false negatives, when a module is not in a diff, but it's dependency is in a diff.

But if we don't use replace directives in go.mod files we don't have this problem. Using tags seems like the more sane way to have a multi-module repo that let's us test components independently and have small scoped PRs.

[robert-zaremba]

What kind of tags do you have in mind?

[tac0turtle]

we should reevaluate how integration tests are done, its unclear if we need to spin multiple tendermint instances in ci to test this. We could do a single node in integration tests making us able to get rid of the test-race jobs

[robert-zaremba]

We want test-race jobs - they are needed to test race conditions.

[tac0turtle]

yea but they are needed cause half of the tests would throw false positives for the race detector. If we got rid of the false positives then we can run everything in race mode

[robert-zaremba]

@marbar3778 are you saying that some tests fail normally but pass with -race?

[tac0turtle]

this was done in line with sonar cloud being added. Now all modules are tested as their own action