fix: github test workflow

[robert-zaremba]

Description

Closes: #10319

@odeke-em reported that db/memdb doesn't build: #10319
The problem is not detected by the Github CI. Here we try to fix this.

---

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
• added ! to the type prefix if API or client breaking change
• targeted the correct branch (see PR Targeting)
• provided a link to the relevant issue or specification
• followed the guidelines for building modules
• included the necessary unit and integration tests
• added a changelog entry to CHANGELOG.md
• included comments for documenting Go code
• updated the relevant documentation 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 ! in the type prefix if API or client breaking change
• confirmed all author checklist items have been addressed
• reviewed state machine logic
• reviewed API design and naming
• reviewed documentation is accurate
• reviewed tests and test coverage
• manually tested (if applicable)

[odeke-em]

Thank you @robert-zaremba for the prompt fix! I believe the problem might be in the xargs being super shallow and not actually invoking go test ./...

I think we don't even need to have pkgs.txt, we just need to invoke go test ./... and that should traverse the entire tree.

[odeke-em]

I would have thought that this would handle the case of db. I think what's missing is the ellipsis for go test ./...

[robert-zaremba]

What do you mean? ./... is there.

[robert-zaremba]

I was running go list ./... locally, and /db package was not there. I was then trying to import it in the main module (/), and it was still not reported by go list ./... - it seams the go list only reports packages in the current module (excluding dependencies)

[odeke-em]

Exactly! What I mean is that instead of trying to invoke go list ./... then getting the name of each package, we just literally remove all of it and just invoke go test ./... as the sole command and we'll be gucci forever, without all the hoops or hardcoding and having packages missed.

[roysc]

I believe go test ./... will also exclude the nested /db module

[robert-zaremba]

go test won't execute db for the very same reason go list won't print it.

@odeke-em we want to have a list of all packages to test in order to execute tests in parallel. We are spawning 4 processes which execute tests.

[robert-zaremba]

other idea would be to find all go.mod files, cd there and rung go list to merge in a single file.

[odeke-em]

Yeah, good idea @robert-zaremba! I think that'll work best because otherwise we have to play catch up to every single Go module that gets added.

[odeke-em]

@robert-zaremba how about this

find . -type f -name "go.mod" | while read F;do dirname $F;done

to list out the packages with go modules in them and then invoke go test ./... on each of them?

[robert-zaremba]

the packages don't have an even test distribution. For example - the SDK has probably 95% of the tests.
Again - we want to put the packages to tests in a matrix.

[tac0turtle]

can you add to the contributing, if a new go.mod is added, this needs to be modified. Otherwise having a script to merge all the files would be useful

[odeke-em]

Thank you for this change @robert-zaremba, almost LGTM, but I had suggested before this pattern

find . -name "go.mod" | grep -v cosmovisor | while read F;do cd $(dirname $F);go list ./... >> $pkgsfile;cd $OLDPWD;done

[odeke-em]

Am not sure about this pattern but as I had suggested before

find . -name "go.mod" | grep -v cosmovisor | while read F;do cd $(dirname $F);go list ./... >> $pkgsfile;cd $OLDPWD;done

should cut the deal properly

[robert-zaremba]

This will do the same effect to what I have implemented. Is there any advantage of your approach?

Unfortunately it doesn't work - see the CI result (I was writing about it somewhere in Discord). Basically, you can't test package of module B while being in module A. For example, the following doesn't work if you are in the repo root: go test ./db/.

I think the only way to fix it is to modify this step:

cat pkgs.txt.part.${{ matrix.part }} | xargs go test -mod=readonly -timeout 30m -coverprofile=${{ matrix.part }}profile.out -covermode=atomic -tags='norace ledger test_ledger_mock'

[robert-zaremba]

We have another problem: from one module we can't test other module. For example, this doesn't work:

cd <cosmos-sdk root>
go test ./github.com/cosmos/cosmos-sdk/db
```

[robert-zaremba]

For the code coverage problem, I think we should merge all profiles in a single file.

[robert-zaremba]

After thinking a little more about it: we are planning to break x/modules in separate go modules -> so the current matrix test won't make sense. Hence, for the sake of the current problem I propose (and going to implement) the following change:

• add a script to test
• add a job to execute all "nested" go module tests together with code coverage (so no in this matrix job).

Later, once we break the SDK in the modules we will remove the current test matrix job and integrate matrix into the module testing job

[codecov]

Codecov Report

Merging #10325 (a621ac0) into master (b5ca8e0) will increase coverage by 0.65%.
The diff coverage is n/a.

❗ Current head a621ac0 differs from pull request most recent head 18ad9cf. Consider uploading reports for the commit 18ad9cf to get more accurate results

@@            Coverage Diff             @@
##           master   #10325      +/-   ##
==========================================
+ Coverage   64.20%   64.85%   +0.65%     
==========================================
  Files         581      594      +13     
  Lines       55206    56380    +1174     
==========================================
+ Hits        35446    36568    +1122     
+ Misses      17742    17725      -17     
- Partials     2018     2087      +69     

Impacted Files	Coverage Δ
x/auth/tx/builder.go	76.05% <0.00%> (-2.55%)	⬇️
types/module/module.go	65.04% <0.00%> (-1.96%)	⬇️
store/cachemulti/store.go	9.52% <0.00%> (-0.48%)	⬇️
crypto/keyring/legacy_info.go	61.95% <0.00%> (-0.41%)	⬇️
crypto/keyring/keyring.go	61.98% <0.00%> (-0.03%)	⬇️
server/init.go	55.55% <0.00%> (ø)
x/mint/module.go	52.54% <0.00%> (ø)
baseapp/baseapp.go	67.11% <0.00%> (ø)
client/keys/rename.go	69.76% <0.00%> (ø)
crypto/keyring/types.go	100.00% <0.00%> (ø)
... and 43 more

[robert-zaremba]

@marbar3778 , @odeke-em , @AmauryM could you make a re-review? I've added a script to handle the problems mentioned above.
There is also #10382 which is an adaptation of the initial implementation I pushed here.

[amaury1093]

lgtm, but utACK. We can merge this, and closely observe a following PR which modifies a submodule.

[robert-zaremba]

closely observe a following PR which modifies a submodule.

I will create a new issue to rework this flow once we will break down the core module into submodules.

[robert-zaremba]

@alessio , @marbar3778, @AmauryM , @aaronc , do you know how to configure the expected tests ? I removed test-cosmovisor and created test-submodules instead.

[aaronc]

@alessio , @marbar3778, @AmauryM , @aaronc , do you know how to configure the expected tests ? I removed test-cosmovisor and created test-submodules instead.

Fixed this now. But I think we should refine this a bit more. Let's discuss in #10446

[robert-zaremba]

Fixed this now. But I think we should refine this a bit more. Let's discuss in #10446

Definitely there is more work to do.