Re-evaluate waiting on green CI

[BridgeAR]

I would like to re-evaluate our rule to have a green CI to land a pull request.

AFAIK the main goal to introduce this was to make sure we have less flaky tests. However, currently it feels like we have at least as many flaky tests as to the introduction point of that rule. Further more, it increases the work for us collaborators and prevents fine PRs from landing.
We probably also landed at least one PR which had a red CI but that could even happen now, when a former CI was green but run before another PR landed which broke something in that PR.

One reason why I see this rule as not working as intended is the new resume CI feature, since that mainly leads to resuming the build until it's finally green instead of checking for actual test failures on the CI (I like the resume CI feature btw. and do not want to see it removed).

So instead of relying on the green CI I would rather rely on the judgement of us collaborators.

Ping @nodejs/tsc

[joyeecheung]

The reasoning makes sense to me, but I would like to see that we require people to post actual analysis of persisting failures before landing things, instead of saying something roughly like "the failures are unrelated" and then go ahead landing PRs.

[Trott]

I strongly oppose this. There is a simple solution: Mark the failing tests as flaky in a status file.

There is typically no reason to land with red CI. We just don't bother marking tests flaky and instead re-run. But if that's too much of a burden, mark the tests as flaky.

[mcollina]

I agree with @Trott.

[Trott]

Recent CI results run by @mcollina (and maybe others) on PRs against release branches strongly suggest that something exists on master only that is causing the raft of CI issues lately.

#25039 (against v10.x branch) and #25036 (against v6.x branch) got green CIs on the first try.

[rvagg]

Fixing flaky tests is unglamorous work, making it easier to avoid that work is just going to make the problem worse. I'd rather go in the opposite direction and increase the pain of flaky tests on contributors so that they can't be ignored and people need to seriously consider stopping their glamorous feature additions and going back and fixing messy stuff they're not going to be celebrated for.

I'd even support a complete code freeze if we hit a certain number of repeating failures, but I doubt that'd be popular!

[misterdjules]

Thinking out loud: what about making CI automatically run stress tests for newly added tests? Would that help decreasing the number of flaky tests added over time? It doesn't solve the problem completely because tests can become flaky over time, but I thought I'd mention it :)

[rvagg]

Can we even automate that? We'd have to inspect commits and see what they touch. Even edits to tests can result in flakies so I guess it'd have to also involve edits to tests. Since the github-bot already inspects code (I think), perhaps it could be extended to leave a comment saying "since you have adjusted tests, please run stress tests on those tests" with some basic instructions on how to do this.

[joyeecheung]

@rvagg The bot only looks at file names, but not the actual content - but I guess it's enough for it to leave the suggestions above

[mhdawson]

I'm also opposed to letting things land with a Red CI. As @Trott mentioned I think the right thing to do is to either re-run or mark the test flaky. I think we should likely also document a requirement to open a new issue if there is not already one for the flaky test in the case of a re-run.

[targos]

@BridgeAR given the feedback, would you accept to close this issue?

[BridgeAR]

Thanks for the feedback. I think opening an issue on failed CIs on a rerun is a good idea (while the flaky tests are often unique).