Optimize tests to run in parallel #1019

inancgumus · 2023-08-31T07:04:13Z

What?

Makes unit and integration tests run in parallel.
Makes some CI optimizations, considering that this is an optimization PR. Let me know if that part deserves another PR.

Why?

The current tests take about 1 minute to run (locally), and there is no reason to run tests sequentially.
This also lets us detect race conditions much faster and prevent unintended deadlocks in code.
The E2E runner is hard to debug when running without the -q flag. With the flag, the result looks clearer to read and debug without the k6 banner and progress bar.

Here are the results after this change (On a 10-core machine):

Before: ~48s
After : ~24s

===Before===
ok      github.com/grafana/xk6-browser/browser  1.853s
ok      github.com/grafana/xk6-browser/common   1.669s
ok      github.com/grafana/xk6-browser/chromium 1.511s
ok      github.com/grafana/xk6-browser/tests    46.902s

===After===
ok      github.com/grafana/xk6-browser/browser  0.956s
ok      github.com/grafana/xk6-browser/common   0.689s
ok      github.com/grafana/xk6-browser/chromium 0.251s
ok      github.com/grafana/xk6-browser/tests    23.150s

On CI, it's the same because our runner runs on a 2-core. We can make this much faster if/when we run it on a more powerful runner.

Checklist

I have performed a self-review of my code
I have added tests for my changes
I have commented on my code, particularly in hard-to-understand areas

Related PR(s)/Issue(s)

Updates #1018

ka3de

Thanks a lot for working on this @inancgumus ! 🙌
Just made a couple of comments. Also, I think there are a few tests missing to be executed in parallel or add a comment explaining why the can't not be run as such.

Unit tests:

TestBrowserContextOptionsPermissions
TestNewBrowserContext
TestConnection
TestConnectionClosureAbnormal
TestConnectionSendRecv
TestConnectionCreateSession
TestContextWithDoneChan
TestPageLocator

Integration tests:

TestLocatorPress

tests/element_handle_test.go

tests/page_test.go

inancgumus · 2023-08-31T10:15:27Z

Thank you for the review 🙇

TestBrowserContextOptionsPermissions, TestConnection, TestConnectionClosureAbnormal, TestConnectionSendRecv, TestConnectionCreateSession, TestContextWithDoneChan, TestPageLocator, TestLocatorPress.

Ah, I missed these ones; thanks! I'll make them parallel.

TestNewBrowserContext

I had sent this PR before our today's discussion in #1007. I was expecting #1007 to use globals, and it wouldn't make sense for this test to be parallel. After the discussion we had, we can make it parallel. WDYT?

ka3de

LGTM 👏

ankur22

Nice! This is awesome! It takes much less time on my machine to run through all the tests.

tests/browser_test.go

common/browser_context_test.go

Before: ~45 sec After : ~20 sec This is measured on my 10-core machine. The results will drastically change depending on the number of cores you have.

This makes tests to run in parallel depending on a CI runner's number of cores.

We can still get the output. Do we need the extra details like a progress bar?

As the only current platform is Ubuntu.

Co-authored-by: ka3de <[email protected]>

After having a discussion, we decided to make this parallel. See the related discussion here: #1007 (comment)

ankur22

Thanks for this 🙂

ka3de

LGTM.

inancgumus self-assigned this Aug 31, 2023

inancgumus added tests dx developer experience optimization labels Aug 31, 2023

inancgumus changed the title ~~Optimize to run tests in parallel~~ Optimize tests to run in parallel Aug 31, 2023

inancgumus marked this pull request as ready for review August 31, 2023 07:22

inancgumus requested review from ankur22 and ka3de and removed request for ankur22 and ka3de August 31, 2023 07:22

inancgumus marked this pull request as draft August 31, 2023 07:28

inancgumus force-pushed the optimize/tests-to-parallel branch 2 times, most recently from d6a47da to fcfbc82 Compare August 31, 2023 08:19

ankur22 mentioned this pull request Aug 31, 2023

Add core to inject k6 object into window #1007

Merged

3 tasks

inancgumus force-pushed the optimize/tests-to-parallel branch 2 times, most recently from 9ea8afe to cdccc4d Compare August 31, 2023 08:41

inancgumus requested review from ankur22 and ka3de August 31, 2023 08:57

inancgumus marked this pull request as ready for review August 31, 2023 08:57

ka3de reviewed Aug 31, 2023

View reviewed changes

tests/element_handle_test.go Outdated Show resolved Hide resolved

tests/page_test.go Show resolved Hide resolved

inancgumus force-pushed the optimize/tests-to-parallel branch from 122818f to 9cd296e Compare August 31, 2023 10:31

inancgumus requested a review from ka3de August 31, 2023 10:52

This was referenced Aug 31, 2023

Fix WebVitals no interaction test #1021

Merged

Optimize tests run speed 2X #1018

Closed

ka3de previously approved these changes Aug 31, 2023

View reviewed changes

ankur22 previously approved these changes Aug 31, 2023

View reviewed changes

tests/browser_test.go Show resolved Hide resolved

common/browser_context_test.go Show resolved Hide resolved

inancgumus added 3 commits August 31, 2023 16:10

Optimize integration tests to parallel tests

2bf0bcd

Before: ~45 sec After : ~20 sec This is measured on my 10-core machine. The results will drastically change depending on the number of cores you have.

Optimize browser package tests to parallel

bbcd2c7

Optimize common pkg tests to parallel

9ad9bb1

inancgumus and others added 7 commits August 31, 2023 16:10

Optimize storage pkg tests to parallel

a632ad6

Update CI tests to run in current parallelism

cff9f65

This makes tests to run in parallel depending on a CI runner's number of cores.

Run E2E tests quietly

02d7133

We can still get the output. Do we need the extra details like a progress bar?

Remove Windows flakiness limiters

d5861b3

As the only current platform is Ubuntu.

Optimize forgotten tests to parallel

62b79c8

Co-authored-by: ka3de <[email protected]>

Make TestNewBrowserContext parallel

da0fb73

After having a discussion, we decided to make this parallel. See the related discussion here: #1007 (comment)

Optimize TestK6Object to parallel

c78e86c

After having a discussion, we decided to make this parallel. See the related discussion here: #1007 (comment)

inancgumus dismissed stale reviews from ankur22 and ka3de via c78e86c August 31, 2023 13:23

inancgumus force-pushed the optimize/tests-to-parallel branch from bdcddb1 to c78e86c Compare August 31, 2023 13:23

ankur22 approved these changes Aug 31, 2023

View reviewed changes

ka3de approved these changes Aug 31, 2023

View reviewed changes

inancgumus merged commit 4fd494f into main Aug 31, 2023
12 checks passed

inancgumus deleted the optimize/tests-to-parallel branch August 31, 2023 13:40

inancgumus added productivity Issues and PRs that improve our productivity and removed dx developer experience labels Oct 6, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Optimize tests to run in parallel #1019

Optimize tests to run in parallel #1019

inancgumus commented Aug 31, 2023 •

edited

Loading

ka3de left a comment

inancgumus commented Aug 31, 2023 •

edited

Loading

ka3de left a comment

ankur22 left a comment •

edited

Loading

ankur22 left a comment

ka3de left a comment

Optimize tests to run in parallel #1019

Optimize tests to run in parallel #1019

Conversation

inancgumus commented Aug 31, 2023 • edited Loading

What?

Why?

Checklist

Related PR(s)/Issue(s)

ka3de left a comment

Choose a reason for hiding this comment

inancgumus commented Aug 31, 2023 • edited Loading

ka3de left a comment

Choose a reason for hiding this comment

ankur22 left a comment • edited Loading

Choose a reason for hiding this comment

ankur22 left a comment

Choose a reason for hiding this comment

ka3de left a comment

Choose a reason for hiding this comment

inancgumus commented Aug 31, 2023 •

edited

Loading

inancgumus commented Aug 31, 2023 •

edited

Loading

ankur22 left a comment •

edited

Loading