Skip to content

Add retry logic for GitHub Actions#599

Closed
tunetheweb wants to merge 13 commits intomainfrom
address-ci-flakiness
Closed

Add retry logic for GitHub Actions#599
tunetheweb wants to merge 13 commits intomainfrom
address-ci-flakiness

Conversation

@tunetheweb
Copy link
Copy Markdown
Member

@tunetheweb tunetheweb commented Mar 21, 2025

Fixes #601

We already test suite twice, but frequently this fails due to flakiness.

This PR adds:

  • change to log beacons to /tmp which uses the in-memory tmpfs so should be less subject to delays.
  • additional logic to the rerun the whole GitHub Action step, which should relaunch the browser as well. Note there is no native GitHub Action logic for this, so have to use an a third-party action (such as this one), of just add a loop. I went for the latter option here.

It doesn't solve the flakiness, but at least automates the reruns I'm doing manually anyway...

@tunetheweb tunetheweb requested a review from philipwalton March 21, 2025 14:45
@tunetheweb tunetheweb mentioned this pull request Mar 21, 2025
@philipwalton
Copy link
Copy Markdown
Member

Could you explain why you changed so many of the tests to wait for complete instead of interactive?

  • change to log beacons to /tmp which uses the in-memory tmpfs so should be less subject to delays.

We could potentially just change the location of that file to be in /tmp if that makes things easier. There's no reason it has to be under /test.

Comment thread test/e2e/onLCP-test.js Outdated
Co-authored-by: Philip Walton <philipwalton@users.noreply.github.com>
@tunetheweb
Copy link
Copy Markdown
Member Author

Could you explain why you changed so many of the tests to wait for complete instead of interactive?

Oh it appears I never hit send on a comment I wrote up for this!!! :-(

So my thinking was interactive says "the document has been parsed but sub-resources such as scripts, images, stylesheets and frames are still loading". So there is a risk that the library has not loaded, or that it's not run anyway at least.

Now I'm not 100% sure about how this works for <script type="module"> (I presume same as defered scripts so potentially later?) and the imports it makes, and I do see you have logic to add __testImport resource loads to __readyPromises but that depends on the script having executed (which as I say I'm not convinced is guaranteed to have happened before interactive).

Additionally, for most of these tests, basically the only thing waiting to complete should be the library load (which we need!) and that should be quick anyway, so didn't see the harm in changing this.

Saying this, I still saw some flakiness with this change, which I can't explain so all of the above could be for naught, but it definitely seemed a bit more stable.

  • change to log beacons to /tmp which uses the in-memory tmpfs so should be less subject to delays.

We could potentially just change the location of that file to be in /tmp if that makes things easier. There's no reason it has to be under /test.

I did think about that, but wondered if there were some Windows people running this, that that would break.

@tunetheweb
Copy link
Copy Markdown
Member Author

Closing this while we see how #603 fares....

@tunetheweb tunetheweb closed this Mar 25, 2025
@tunetheweb tunetheweb deleted the address-ci-flakiness branch May 7, 2025 19:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Flakey tests

2 participants