ci: Add test retry logic for flaky tests

[dplewis]

Pull Request

• Report security issues confidentially.
• Any contribution is under this license.
• Link this pull request to an issue.

Issue

Flaky tests require a lot more effort to merge PRs. Test would have to be re-ran until the flaky tests passes or they could be ignored if you know which tests are flaky. Ideally every flaky test should be fixed as they are found which hasn't been the case.

Closes: #8654

Approach

• Add the ability to retry well known flaky tests (tests that randomly fail)
• Identify test by name
• Retries 3 times if flaky test fails
• Remove function scoping this from test suite RegexVulnerabilities.spec

[parse-github-assistant]

Thanks for opening this pull request!

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 93.76%. Comparing base (453a987) to head (e6b6fd1).
Report is 5 commits behind head on alpha.

Additional details and impacted files

@@           Coverage Diff           @@
##            alpha    #9218   +/-   ##
=======================================
  Coverage   93.76%   93.76%           
=======================================
  Files         184      184           
  Lines       14715    14715           
=======================================
  Hits        13797    13797           
  Misses        918      918           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[mtrezza]

Could you explain how this works?

[dplewis]

Override the Jasmine Spec class, replace the original test function with retrying the original test function

[mtrezza]

How are flaky tests identified, so they get fixed at some point? What's the purpose of flakyTest.json?

What's the implication of "Identify test by name"? What happens if 2 tests have the same name?

[dplewis]

How are flaky tests identified, so they get fixed at some point? What's the purpose of flakyTest.json?

From this point forward any test that randomly fails for no reason is a flaky test. If you find such a test add it to flakyTest.json

What's the implication of "Identify test by name"? What happens if 2 tests have the same name?

If any test with that name fails will get retried. The only implication is if the name gets changed. If the name gets changed hopefully they are fixing the flaky test.

I could set it up to auto retry any failed test. The only problem with that is local development but I can set it up to retry on CI only.

@mtrezza You are the main reviewer so whats easier for you? Do you currently keep track of randomly failing tests or just rerun everytime?

[mtrezza]

If we undertake a targeted effort to fix flaky tests we track them via an issue, like we did in #7180. There is currently no such open issue where we actively track them, but I don't think that's necessary as we have significantly less flaky tests today than we had back then - also thanks to your efforts.

Why don't we use test IDs to identify flaky tests? If we see a test is flaky, we assign an ID via it_id and then add it to the flaky list, similar to how the test exclusion list works.

[dplewis]

Why don't we use test IDs to identify flaky tests? If we see a test is flaky, we assign an ID via it_id and then add it to the flaky list, similar to how the #8714 works.

I tried that already. I can't get the id as it happens before the test spec is ran and there is no way to pass it through.

[mtrezza]

For example, in https://github.com/parse-community/parse-server/actions/runs/9983595053/job/27591447103 I would identify this flaky test:

1. ParseLiveQuery handle invalid websocket payload length

• Error: Timeout - Async function did not complete within 200000ms (set by jasmine.DEFAULT_TIMEOUT_INTERVAL)

So I'll assign ID 4ccc9508-ae6a-46ec-932a-9f5e49ab3b9e to it in #9205.

And I would add that ID to the flaky list.json, right?

-- Edit, just read your previous comment; that's too bad. Maybe we can modify the it_id in the future so that the ID becomes accessible. And we use the names for now. I just don't think we will / want / can enforce that names won't change, because of typos, rephrasing, reorganizing, etc, so that's a weak spot we'd accept for now.

[dplewis]

You would add ParseLiveQuery handle invalid websocket payload length to the list also why is that timeout so long? 200000ms?

[mtrezza]

It's idempotency where we simulate a TTL index:

/** Enable TTL expiration simulated by removing entry instead of waiting for MongoDB TTL monitor which
runs only every 60s, so it can take up to 119s until entry removal - ain't nobody got time for that */

Haven't looked at this for a while but I vaguely remember that I've set it to that value on purpose; it has something to do with the comment above.

[mtrezza]

At what point should we turn on the test randomizer again?

[dplewis]

At what point should we turn on the test randomizer again?

Jasmine 5.0.0 released Running Spec in Parallel that would require randomizer to be turned on. To upgrade Jasmine we would have to give the test suite some TLC as we are mixing async and done()

[mtrezza]

Looks good

[mtrezza]

But we had the randomizer turned off for flakiness investigation, right? So if we now fixed a lot of these flaky tests, plus we have this retry logic, shouldn't we turn it back on?

[dplewis]

Yeah we can turn it back on in a separate PR.

[mtrezza]

Isn't turning on the randomizer part of this retry strategy? It would also be interesting to see how this tool behaves with the randomizer turned on.

Before we merge this we should find a process for how to use this tool, what's the criteria for adding a test to the list and how to deal with flaky tests once they are obscured by this tool. For example, there is flakiness that isn't related to a specific test but is a result of previous tests. So we may end up adding more and more tests to the flaky list without really solving anything.

[mtrezza]

Is there any way of knowing, how many retries are required for the flaky tests to pass? Some stats would be interesting to know how efficient the approach is.

[dplewis]

@mtrezza It now outputs how many times a flaky test retried to pass

[mtrezza]

Looks good to me. As I mentioned earlier, I'm still somewhat unsure how this tool is intended to be used. A flaky test may well indicate a bug and not just be a test issue. So we'd need to define when to add a test to the list.

Adding a test to the list for retrying only makes sense if we work on fixing them once they are added, otherwise we are effectively weakening our CI quality. The annoyance of a failing CI is what drove fixing flaky tests in the past. This tool creates the convenience of hiding the flakiness, but that may undermine the drive to fix it.

So roughly, the rules could be:

• A test is flaky if it sometimes fails on our (pre-)release branches, not in a PR branch, as the flakiness may be a result of the changes in the PR that is still under development.
• After adding a test to the flaky list, a GH issues needs to be opened to fix the test so that it gets removed from the list as soon as possible.

[mtrezza]

@dplewis how should we treat this test issue?

https://github.com/parse-community/parse-server/actions/runs/10028582673/job/27715676644#step:8:1

It seems that after shutdown LiveQuery the subsequent tests all failed. This looks like an issue with the test logic, so we wouldn't add anything to the flaky list, right?

[parseplatformorg]

🎉 This change has been released in version 7.3.0-alpha.7