Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Separate web testing into per-browser runners #17529

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

torokati44
Copy link
Member

Just pushing to see whether it will work.

@Dinnerbone
Copy link
Contributor

The errors suggest that the built ruffle wasn't found

@torokati44
Copy link
Member Author

Anyone, feel free to force-push while I'm busy, if you'd like to experiment, I don't mind.

@Dinnerbone
Copy link
Contributor

I fixed the failure with running the test.

Some comments that I don't have time to fix:

  • We should add an argument to set wdio:maxInstances, not blanket set it. It hammers a computer if you run it all tests all browsers, and it also muddies browserstack. Something like --parallel maybe, to change it from 1 to 5.
  • Only one of the "test web / ..." needs to actually build this image and kick it off. I super duper don't think it's likely that a test in a browser will fail because of a node version or OS.

@torokati44
Copy link
Member Author

I fixed the failure with running the test.

Yay, thanks! ^^

Only one of the "test web / ..." needs to actually build this image and kick it off. I super duper don't think it's likely that a test in a browser will fail because of a node version or OS.

Uh, building what image...? 🤔 This is an entirely separate matrix from the build job. We can cut it down of course, I was just curious about the whole space initially.

@Dinnerbone
Copy link
Contributor

Dinnerbone commented Aug 16, 2024

Uh, building what image...? 🤔 This is an entirely separate matrix from the build job. We can cut it down of course, I was just curious about the whole space initially.

The dist folder, sorry.

So right now it's:

  • Node 20 / Windows
    • triggers Node 20 / Windows / Chrome
    • triggers Node 20 / Windows / Edge
    • triggers Node 20 / Windows / Firefox
  • Node 20 / Ubuntu
    • triggers Node 20 / Ubuntu / Chrome
    • triggers Node 20 / Ubuntu / Edge
    • triggers Node 20 / Ubuntu / Firefox
  • Node 22 / Windows
    • triggers Node 22 / Windows / Chrome
    • triggers Node 22 / Windows / Edge
    • triggers Node 22 / Windows / Firefox
  • Node 22 / Ubuntu
    • triggers Node 22 / Ubuntu / Chrome
    • triggers Node 22 / Ubuntu / Edge
    • triggers Node 22 / Ubuntu / Firefox

But it should just be the 3 browsers, triggered by one of the web tests. Maybe ubuntu node 22.

Something like:

  • Node 20 / Windows
  • Node 20 / Ubuntu
  • Node 22 / Windows
  • Node 22 / Ubuntu
    • triggers Chrome
    • triggers Edge
    • triggers Firefox

@torokati44
Copy link
Member Author

But it should just be the 3 browsers, triggered by one of the web tests. Maybe ubuntu node 22.

Sure - but I don't think this is how it works...? As in, they don't trigger each other, they all are triggered when the workflow starts, there is just a blocking dependency among some of them that holds them up for a while. At least this is how I see it.

@Dinnerbone
Copy link
Contributor

Just change the matrix of browser-tests to only 3 entries, and have them all download from the same source. Then, in the build job, only have that blessed one do the upload, to save time there too.

@Dinnerbone
Copy link
Contributor

(sorry I forgot how silly github workflows are for a bit)

@torokati44
Copy link
Member Author

And I suspect we'll have to adjust that heckin' fake no-op job as well, if we want to make these jobs required aswell/instead.

@torokati44
Copy link
Member Author

torokati44 commented Aug 16, 2024

image

I wish it was possible to make the browser tests wait only for the ubuntu builds... it would be so much quicker. But alas... :/
https://stackoverflow.com/questions/74881221/require-only-one-specific-job-in-matrix-to-finish-for-other-dependent-job
https://github.com/orgs/community/discussions/42335

We could split it (and for example, remove the formatting checker step from the windows ones), but eh, that would be a bit too much copy-paste...

@Dinnerbone
Copy link
Contributor

it would be so much quicker

Well, it seems that the full "test web" takes exactly as much time as before - so at least it's not slower? :D

We could split it (and for example, remove the formatting checker step from the windows ones), but eh, that would be a bit too much copy-paste...

There is a kind of template/reuse system in workflows. It's all blah, but it exists.

Alternate idea: have one task that's "build ruffle web", copy its dist folders over as an artifact into the existing "test web" matrix which then does the usual npm run test.

@torokati44
Copy link
Member Author

There is a kind of template/reuse system in workflows. It's all blah, but it exists.

Oooohhh.... 👀 BTW, just remembered, YAML itself is supposed to have aliases and anchors, I wonder whether those work here...

Alternate idea: have one task that's "build ruffle web", copy its dist folders over

Yeah that could work, but then we wouldn't be testing building on different platforms, which I thing is valuable.

@Dinnerbone
Copy link
Contributor

YAML itself is supposed to have aliases and anchors, I wonder whether those work here...

In my experience, basically nothing supports it properly, if at all :D Github is no exception

Yeah that could work, but then we wouldn't be testing building on different platforms, which I thing is valuable.

Well what about the wasm at least? We'd need to refactor the build script to take a prebuilt wasm but it should be possible. That should not depend on node or OS, unless something is seriously wrong with a dependency (like rust itself, or maybe bindgen), and it's the slowest part by far.

@Dinnerbone
Copy link
Contributor

... maybe that's for a future PR though? :D

@torokati44
Copy link
Member Author

In my experience, basically nothing supports it properly, if at all :D Github is actions/runner#1182

blaargh....

Well what about the wasm at least? We'd need to refactor the build script to take a prebuilt wasm but it should be possible. That should not depend on node or OS, unless something is seriously wrong with a dependency (like rust itself, or maybe bindgen), and it's the slowest part by far.
... maybe that's for a future PR though? :D

Sure, makes sense.

What do you think about the required checks thingy?

And I still have to adjust the parallelism count stuff, but other than that, does this look good?
Related - just to confirm, we don't run BrowserStack tests on CI, right?

@Dinnerbone
Copy link
Contributor

What do you think about the required checks thingy?

Not much to think about it other than "it needs to work", these should be required checks yes :D

And I still have to adjust the parallelism count stuff, but other than that, does this look good?

I think so!

Related - just to confirm, we don't run BrowserStack tests on CI, right?

Correct but not for lack of wanting; just haven't got to it yet.

@torokati44 torokati44 force-pushed the split-browser-tests branch 2 times, most recently from 4cd905c to b39ceea Compare August 22, 2024 13:14
@danielhjacobs danielhjacobs added A-tests Area: Tests & Test Framework T-refactor Type: Refactor / Cleanup labels Sep 17, 2024
@torokati44 torokati44 force-pushed the split-browser-tests branch 2 times, most recently from 9c241c3 to 589cde1 Compare September 18, 2024 02:26
@torokati44
Copy link
Member Author

Note again that, currently, this takes longer than before, because the browser tests also have to wait for the sluggish Windows builds to finish.

I'm now seriously considering splitting them into their own little "matrix of shame for pathetically slow OSes": actions/runner-images#7320

At least we could really easily tell which browsers fail/hang/lag/etc...

@torokati44
Copy link
Member Author

I suppose, I could still look at adding that --parallel option, to see...
And even fake the needs behavior with some sort of "wait until artifact is available" hack... 😬

@torokati44 torokati44 force-pushed the split-browser-tests branch 4 times, most recently from 95f8793 to 3e7328e Compare September 24, 2024 18:02
@torokati44 torokati44 force-pushed the split-browser-tests branch 2 times, most recently from 9c010d4 to c6de3d3 Compare October 2, 2024 09:49
@Dinnerbone
Copy link
Contributor

Okay sorry to keep holding this up but

Testing the build on diff platforms is valuable, I agree
But blocking this matrix on builds on diff platforms isn't valuable, as I'm sure you agree

Why not a separate and fast "build web for testing" which triggers the new jobs, unrelated to the existing "build and test on X"?
And then in the future we can reuse that for the browserstack tests too

@torokati44
Copy link
Member Author

Why not a separate and fast "build web for testing" which triggers the new jobs, unrelated to the existing "build and test on X"?
And then in the future we can reuse that for the browserstack tests too

Oh, this is a nice way to phrase "separate matrix for slow-as-heck platforms", I like it!

@torokati44 torokati44 force-pushed the split-browser-tests branch 3 times, most recently from 843a98c to 17e597c Compare October 9, 2024 23:48
@torokati44
Copy link
Member Author

Why not a separate and fast "build web for testing" which triggers the new jobs,

Done, with a whole bunch of copy-pasting. :/
The slowest job is now again Rust on Windows - the order of the world has been restored... 😌

@torokati44
Copy link
Member Author

Note that this needs some attention when rebasing, to account for recent changes. If done automatically, it will break things.

@torokati44 torokati44 force-pushed the split-browser-tests branch 2 times, most recently from 08923a1 to e123990 Compare October 19, 2024 15:17
@torokati44
Copy link
Member Author

Manual rebase correction done, I hope I haven't missed anything, unless @danielhjacobs objects, I guess this is good to merge now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-tests Area: Tests & Test Framework T-refactor Type: Refactor / Cleanup
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants