search

mozilla / mozperftest-tools

Mozilla Public License 2.0
4 stars 13 forks source link

Enable side-by-side for interactive browsertime tasks with videos. #60

Closed gmierz closed 10 months ago

gmierz commented 11 months ago

This patch adds the ability to run side-by-sides for interactive browsertime tests (and any other tests with produce videos). One caveat is that there needs be an overlap between the alias in the browsertime.json entry, and the test name found within a perfherder-data.json suite. A few methods needed to be fixed to handle non-cold/warm tests as well.

gmierz commented 11 months ago

Tested locally on the mozilla-central side with: ./mach perftest-tools side-by-side --new-revision b3d4dd6d5b436ace9de5430fd3ebdd2869014ce8 --base-revision 72153d629a084254f7067b3eeb48181de01952f7 -t browsertime-responsiveness-firefox-reddit-billgates-post-2 --base-platform test-windows10-64-shippable-qr/opt

alexandru-io commented 11 months ago

I was planning to look into it and make a coding jam-session to enable side-by-side for interactive. That's awesome! I will look into it ASAP

gmierz commented 11 months ago

Ah ok! I needed a side-by-side for this bug so I spent a bit of time getting it running: https://bugzilla.mozilla.org/show_bug.cgi?id=1856527

Something to note is that we need a better way of determining if we're looking at a cold/warm pageload test, or a custom one (currently using the task name). Also matching the perfherder-data suites with the browsertime data is flimsy since there's no guaranteed 1:1 relationship between these (luckily the alias is used in the test name for interactive tests).

alexandru-io commented 11 months ago

I would rename this patch to "enable side-by-side for interactive [pageload] sites". There's a lot of browsertime tasks that side-by-side doesn't work for: benchmark, custom, ps.

gmierz commented 11 months ago

Thanks for the review! I've resolved the conflict, and I've updated the patch name to specify only browsertime tasks with videos.

gmierz commented 11 months ago

I've set it to interactive because there's another thing that this patch doesn't handle which is tests that don't set a test alias (all of ours do).