🔪 flaky and Zombienet tests

[ggwpez]

Commenting out all flaky tests and tracking them here #48

Changes:

• Disable flaky Rust tests by adding a new disabled feature. #[ignore] attribute is not possible since CI runs with --ignored

• Disable all Zombienet tests

• Waiting for CI what other tests fail.

[paritytech-workflow-stopper]

All GitHub workflows were cancelled due to failure one of the required jobs.
Failed workflow url: https://github.com/paritytech/polkadot-sdk/actions/runs/15184421552
Failed job name: fmt

[ggwpez]

Initially this was disabling the flaky ones, why all zombienet tests now? Some of them were passing afaik.

Yes but i kept disabling more and more, so now I disabled all since there is no way to check if they are not flaky without running the CI many times.

Maybe we can go the other way and initially disable all and then enable the non-flaky ones one-by-one.

[bkchr]

Maybe we could just disable the zombienet tests in the merge queue?

[athei]

Maybe we could just disable the zombienet tests in the merge queue?

In the PR they are just noise. We always merge with them red because they almost always are. I see no reason to keep them.

[alindima]

I see this PR disabled all zombienet tests, which I think was a bad idea.
They aren't just noise. Certainly not all of them were flaky and most of them were flaky for infrastructure reasons (running on spot instances or undersized hardware), based on my experience of running them locally to check. Us in the Parachains team have been working on porting them to zombienet-sdk and trying to make them more reliable.

I'd suggest to just not make them mandatory for merging the PR. We should still run them but take the results with a grain of salt. There are certainly some tests that are failing almost all the time (and we could disable these altogether maybe), but at least we also had plenty of tests that were failing extremely rarely (or never).
They are valuable even as informational only. Maybe not for everyone, depending on which area of the code you're usually touching

[bkchr]

Us in the Parachains team have been working on porting them to zombienet-sdk and trying to make them more reliable.

Porting to zombienet-sdk will not change anything in regards to their flakiness.

They are valuable even as informational only.

No. we already have merged prs in the past with failing tests and then realizing that they actually broke. It is also not really easy too check them etc.

The only proper fix is to make them rock stable. Stuff like spot instances need to be detected by zombienet itself and not counted as an error. But we also had this discussion already multiple times and these tests are still flaky and it doesn't improve.

[alindima]

Porting to zombienet-sdk will not change anything in regards to their flakiness.

We don't only do a direct translation. We try to test them thoroughly and investigate their flakyness. zombienet-sdk sometimes enables us to make more precise assertions.

No. we already have merged prs in the past with failing tests and then realizing that they actually broke. It is also not really easy too check them etc.

Right, there's a chance of merging some buggy code. But now we can almost be sure that we'll merge some buggy code

[alexggh]

The only proper fix is to make them rock stable.

I don't think anyone disagrees here, but are we in a better situation with all of them disabled ?

[pepoviola]

Hi, I'm also agree on this

The only proper fix is to make them rock stable.

And we are working to reach that stability, just as update:

• We are working on re-enable the dashboard (tracking tests and make it easy to visualize them)
• Split the deployment and test phases to easily categorize (and re-try) tests when the deployment phase fail (related to infra issues).
• Audit failing test , get the root cause (infra or logic related) and fix them.

Also, in order to get more information of the test but without causing friction we propose to only run them when certain label is present (so people form parachains/node sdk can run them easily) and also make them not required.

Any other feedback/opinion is wellcome.

[athei]

I'd suggest to just not make them mandatory for merging the PR. We should still run them but take the results with a grain of salt.

They were never mandatory. That would have been insane. But you still had to wait for them to finish running. And I am not checking the results and nobody is checking the result of a job that fails > 50% of the time. I just merge just as anybody else. They provide ZERO value.

It makes no sense to have optional tests. Just make them solid or don't add them. We need to do better. But until then they just don't run in a PR.

[alindima]

They were never mandatory. That would have been insane. But you still had to wait for them to finish running. And I am not checking the results and nobody is checking the result of a job that fails > 50% of the time. I just merge just as anybody else. They provide ZERO value.

I for one (and I assume others too) got a good memory of which particular tests are flaky (or at least you could check on other PRs which are the most common ones). Definitely not all of them are (especially to the 50% ratio that you mention).
If I'm writing a particularly large PR that adds a lot of code it's useful to have the CI run and for the couple of flaky tests that are reported I'd run them locally to make sure they are passing.
Me and others have been doing this for a while. It's frustrating but certainly better than just not running any of them.

I can understand that this is not valuable to everyone, but for some it is. Having the label is a good compromise for the time being. At least I can test my code and the code I'm reviewing.

[ggwpez]

I can understand that this is not valuable to everyone, but for some it is. Having the label is a good compromise for the time being. At least I can test my code and the code I'm reviewing.

Yea lets go with the label for now.

I for one (and I assume others too) got a good memory of which particular tests are flaky (or at least you could check on other PRs which are the most common ones).

But to all other people working on Polkadot SDK it is confusing AF. This is a team project!
New developers come in and think they made a mistake and get frustrated because CI is red.
Disabling all ZN tests is my way of escalating the issue since it has not been taken seriously for the last two years!

[alexggh]

Disabling all ZN tests is my way of escalating the issue since it has not been taken seriously for the last two years!

You should've made it the other way around make them mandatory :D, so that everyone stops and fix them, but I guess we tried that as well :D.

Anyways, let's go with the label for now, but my instinct tells me this has the reverse impact where they get even lower priority.