garris / BackstopJS

Catch CSS curve balls.
http://backstopjs.org
MIT License
6.69k stars 604 forks source link

Reuse playwright browser #1417

Closed FrostyShosty closed 2 years ago

FrostyShosty commented 2 years ago

This is a possible fix to #1416. Using James Irish's idea to reuse the playwright browser, this code only creates the browser once, and creates independent browser contexts for each scenario, safely closing the browser at the end.

Passes smoketests (relatively?) quickly, but I do not have his 624-scenario setup to truly test the speed.

JamesIrish commented 2 years ago

Thanks for this, just carry out some testing and I think runPlaywright.js line 257 (call to delegateSelectors) needs changing from browser to browserContext as it seems the browser instance is getting closed too early - I can't seem to comment on that line in the diff for some reason..

JamesIrish commented 2 years ago

Results are in (have taken copies of your edited files and overwritten my node_modules) these timings are all for our full 624 scenario run.

Spec is one of either: Wk = Workstation, my local work setup, Intel i7 but lots of security software! 😞 Vm = Our own Azure DevOps build agent VM (B2ms, 2vCPU, 8GB RAM)

# Spec With fix Using Docker Time
1 Wk No No 38 minutes, 27 seconds
2 Wk Yes No 8 minutes, 46 seconds 🥇 🚀
3 Wk No Yes 10 minutes, 41 seconds
4 Wk Yes Yes 10 minutes, 4 seconds
5 Vm No No 8 minutes 31 seconds
6 Vm Yes No 7 minutes dead
7 Vm No Yes 8 minutes 15 seconds
8 Vm Yes Yes 8 minutes 13 seconds

The change makes it quicker every time ✔️ though I've absolutely no idea why my work device is soooo much slower when repeatedly calling playwright.launch. I'll take a look into what that method's doing under the hood at some point this week.

I'd vote to get this in, at least to a beta release for further testing.

garris commented 2 years ago

Ok, this sounds like a great optimization. I skimmed the PR and it LGTM. I'll keep an eye out for any fast follows. Thanks all!

@FrostyShosty @JamesIrish