ci: run most tests all the time, again

[thehowl]

#2531

following #3316

Morgan:
https://github.com/gnolang/gno/blob/master/.github/workflows/gnovm.yml
Can we clarify one thing once and for all:
I think the CI should run, even on pull requests, all tests, all the time, because that is the point of the monorepo.
It is not enough to test tm2 and gnovm when we change the gnovm, but gno.land should be tested as well, and so should be gnodev, gnobro and all other tools, because they always depend on the latest master, and if I break a function definition on gnovm, I need to change it in all other places as well; and the CI should be red

We changed this behaviour a few times already. I don't want to see a CI that succeeds in a PR, but fails on master. We shouldn't try to make the CI run "select workflows" in PRs so that it's green faster; we should make the slow workflows fast. There are no shortcuts to making them fast, it means going into the codebase armed with pprof to understand what the hell is wrong, but that's what I've done with the gnovm tests and started doing with the gno.land tests as well (see: #3327 ). I think we should focus more on this than other approaches which are band-aids on the problem (like: making the machines bigger, or running the tests in parallel, or running workflows selectively).
I'm not confident merging this PR: #3341
because no gno.land tests are executed, and we've had situations before where adding new packages or changing things in examples/ broke gno.land. (So no, even trying to filter running tests on code changes specifying *.go is error-prone)

Milos:
I don’t understand where in this process you don’t run the local testing suite?
Why wait for the CI to tell you things are actually broken?

Morgan:
The CI should be more correct than a human in detecting a broken build
The CI should help me, as a reviewer, in assessing that merging this PR has no fatal consequences on the master branch

Milos:
Yes, but it doesn’t mean we shouldn’t run the local suite

If the CI suites don’t cover everything, it’s indicative of the tests we have, not specifically the CI on its own (logically if the > separation makes sense, but IRL it starts failing, we might have a different problem)

Running everything doesn’t solve the issue of bad testing suites

Sergio:
Isn't there a more fine grained condition that allows meet halfway?
@ Morgan your point are good, but on the other hand IMHO we should rely on a CI that does not last 3 hours for each commit

Morgan:
The fix is to make the gno.land CI go from 20 minutes to 5

Guilhem:
In my opinion, CI should be the single source of truth. We should not trust people to run local tests on their machines, especially because we are an open-source project and have external contributors.

Nathan:
I see this as a "yes, and" problem, not an either or. The CI's "long tail" run should be faster AND it should only run what is provably needed on any given PR.

Morgan:
The tests in gnovm shouldn't verify that the usages in gno.land keep being correct
Consider my previous example: if Machine.BeginTransaction begins accepting a new argument (as recently happened), the CI should verify that the usages in gno.land are correct, because we are using a monorepo and gno.land will always use the "gnovm latest", not a specific version

We are in a monorepo nathan. If gno.land was a separate repo from gnovm, you'd run tests on a dependency bump on the gnovm because you'd want to make sure the update broke nothing
With the monorepo, gno.land always uses the latest version of the gnovm. That is good, and desired.
But it carries the consequences that we always need to test gno.land as a consequence.

Nathan:
gnovm would be in the dependency graph of lots of things, so it absolutely should rerun things existentially. I think we're both saying the same thing here.

It also signals to the developer how "far reaching" a change is. Anyone working in gnovm knows this would happen a lot, but forcing a gnovm build workflow on other parts of the dependency graph is anti-social and expensive from a build process and promotes resistance to changing things.

Morgan:
Note that I'm using this dependency as an example, but this goes also for other dependencies (like gnodev depending on gno.land, or gno.land depending on tm2)

I think you mentioned some tools that would allow us to test selectively @ Nathan Toups by analyzing dependency graphs. I can understand those, and we can implement them and allow us to scale the monorepo to even bigger. And they're not shortcuts; they allow me to actually verify that the PR is green, and so would be master.

Until we have them (which is non-trivial, also because it's not just direct go dependencies we need to analyze, but also a dependency of the gno.land tests with the examples directory), I rest my case we should test everything all the time.

Guilhem:
In my opinion, CI should be the single source of truth. We should not trust people to run local tests on their machines, especially because we arehttps://github.com//pull/3390#issue-2752980428 an open-source project and have external contributors.

It doesn't matter to me if the build runs for less than 30 seconds. The current main problem is the time it takes to run some tests, not the fact that we run those tests.

Milos:
I still don’t understand what running “make test” doesn’t catch locally, if this is the concern for this example specifically

The CI isn’t our only safety net — we should have layers until something hits master

The CI can always be improved more, PRs and issues are always welcome, and new approaches especially — let’s discuss

Morgan:
Don't consider the PR author workflow, but consider the reviewer workflow
as a reviewer, I should be able to trust the CI telling me that the changes are good to be merged. I shouldn't have to always check things out locally, for instance for adding a new package to examples/, and running the testing suite for the whole repository. We have automated tools that check that, like the CI, and we should use it.
I should also remind that if you're 100% confident that a change doesn't break master, nothing forbids you from merging a PR with a running gno.land CI suite, because we don't have a requirement on the CI passing to merge, it's a social norm.
I realise now that my example was a bit inadequate, because double-checking my logic against the workflows, gno.land tests would be run with changes on gnovm. I think the point as a whole is still valid:

• because contribs/ are not tested for changes in gno.land
• because gno.land is not tested for examples/
• because how we're laying out dependencies is being done manually rather than with automated tooling which can detect at least 99% of package and module dependencies and essentially be "smarter" than us, and I see it as such as being error prone

Sorry I got emotional on this one, I'm a bit frustrated by the back-and-forths on the topic and by attempts to "shortcut" a slow CI while I've put considerable effort in making our CI and tools suck less by actually finding the root causes of slowness; and I would appreciate if we moved to also doing this elsewhere (ie. gno.land tests) rather than trying to add other potentially dangerous band-aids.

[Gno2D2]

🛠 PR Checks Summary

All Automated Checks passed. ✅

Manual Checks (for Reviewers):

• IGNORE the bot requirements for this PR (force green CI check)

Read More

🤖 This bot helps streamline PR reviews by verifying automated checks and providing guidance for contributors and reviewers.

✅ Automated Checks (for Contributors):

🟢 Maintainers must be able to edit this pull request (more info)

☑️ Contributor Actions:

1. Fix any issues flagged by automated checks.
2. Follow the Contributor Checklist to ensure your PR is ready for review.
   • Add new tests, or document why they are unnecessary.
   • Provide clear examples/screenshots, if necessary.
   • Update documentation, if required.
   • Ensure no breaking changes, or include BREAKING CHANGE notes.
   • Link related issues/PRs, where applicable.

☑️ Reviewer Actions:

1. Complete manual checks for the PR, including the guidelines and additional checks if applicable.

📚 Resources:

• Report a bug with the bot.
• View the bot’s configuration file.

Debug

Automated Checks

Maintainers must be able to edit this pull request (more info)

If

🟢 Condition met
└── 🟢 The pull request was created from a fork (head branch repo: thehowl/gno)

Then

🟢 Requirement satisfied
└── 🟢 Maintainer can modify this pull request

Manual Checks

**IGNORE** the bot requirements for this PR (force green CI check)

If

🟢 Condition met
└── 🟢 On every pull request

Can be checked by

• Any user with comment edit permission

[zivkovicmilos]

Why remove?

[thehowl]

autocounterd binary depends on gnoclient, vm, rpc/client, commands, so if those change this needs to change too

[thehowl]

this is just for the docker build, so i'm actually ok in reverting this seeing as it's otherwise tested in misc.yml

[zivkovicmilos]

So if we merge a single md change, we need to run the benchmarks?

[thehowl]

we can make this **/*.go

[thehowl]

also, this benchmark runs in like < 1 minute now

Why do you care about the number of CI checks being executed, especially on master? Personally, I only care about the time it takes for the whole CI to run. (It's not rhetorical, it's a genuine question to understand if I'm missing something.)

[zivkovicmilos]

Why would we not run them for just these changes?

What does a change in any other part of the codebase have to do with the examples? I'd like to see real examples of this issue

[thehowl]

#2491 was green in the PR, but was red after being merged, because a txtar test was no longer correct.

[zivkovicmilos]

Why?

[thehowl]

We are working in a monorepository because we have many different components that depend on each other and a codebase that will likely still be in turmoil for the upcoming years. Even remaining on the "simplest" level of breakages, API changes, components in misc/ and contribs/ like autocounterd, loop, gnodev, and gnogenesis depend on the APIs exposed by the many packages in gno.land, gnovm and tm2. If any of these change, and the build breaks on any of these components, the CI should be red, and the issue fixed immediately in the same PR as the API change.

But aside from API breakages, components can change in all kinds of subtle ways that their usages elsewhere might not expect. That's why we have tests to ensure that things keep working and behaving as we expect them, and why if tm2 changes, the entire repository needs to be tested, and I don't see many ways out of that.

[zivkovicmilos]

I'd say find the dependency, and add it as a path requirement

[thehowl]

I would say just about the entire repository; maybe an exception for tm2/*.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

📢 Thoughts on this report? Let us know!