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

[test] Mock getComputedStyle to speed up unit tests #4142

Merged
merged 6 commits into from
Mar 11, 2022

Conversation

m4theushw
Copy link
Member

@m4theushw m4theushw commented Mar 9, 2022

Related to #3983

The implementation of Window.getComputedStyle provided by JSDOM parses all stylesheets in the page to find only the rules that affect the given element. This makes it very slow when compared to the version provided by a real browser. After investigation, I noticed that we're calling it in at least two places but none of these calls are necessary, taking into account the kind of tests we do:

const computedStyle = win.getComputedStyle(parentElement.current);
const paddingLeft = parseInt(computedStyle.paddingLeft, 10) || 0;

var elementStyle = hostWindow.getComputedStyle(element);
if (elementStyle && elementStyle.position == 'static') {
element.style.position = 'relative';

The first call is used to get the paddings applied to the grid container, but no test sets padding. The second one is only used to set position=relative if it was position=static, which never happens. We can save some milliseconds by not calling this function. Also, all *ByRole queries relay on it to check if the element is hidden or not, but we only use CSS to hide an element in the filter form.

sx={{
display: hasLinkOperatorColumn ? 'flex' : 'none',

With that being said, the purpose of this PR is to change to run the unit tests with a mocked getComputedStyle that does nothing. The trade-off of using a mock is that toHaveComputedStyle won't work, so either run it in a real browser or assert manually the styles. Another issue is that the *ByRole may return different number of elements between JSDOM and a real browser if the element is hidden by CSS.

Before: (taken from 09c0d35)

image

After: (taken from 8956bdd)

image

@mui-bot
Copy link

mui-bot commented Mar 9, 2022

These are the results for the performance tests:

Test case Unit Min Max Median Mean σ
Filter 100k rows ms 263.4 589.2 334.6 396.84 136.312
Sort 100k rows ms 424.7 936.2 717.6 715.84 166.819
Select 100k rows ms 115.6 288.1 125.7 166.24 65.239
Deselect 100k rows ms 112.2 197.2 167.6 157.6 28.463

Generated by 🚫 dangerJS against 485181a

@m4theushw m4theushw marked this pull request as ready for review March 9, 2022 22:27
@m4theushw m4theushw changed the title [test] Attempt to make unit tests run faster [test] Mock getComputedStyle to speed up unit tests Mar 9, 2022
@m4theushw m4theushw added the test label Mar 9, 2022
@m4theushw m4theushw self-assigned this Mar 9, 2022
@m4theushw m4theushw requested a review from a team March 9, 2022 23:46
Copy link
Member

@cherniavskii cherniavskii left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we run unit tests in real browsers, is there any value in running same tests in jsdom?
What if we skip all tests in jsdom except those that are skipped in real browsers?

@@ -10,6 +10,16 @@ require('@babel/register')({

createDOM();

// The JSDOM implementation is too slow
// https://github.com/jsdom/jsdom/issues/3234
window.getComputedStyle = function getComputedStyleMock() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@m4theushw m4theushw merged commit 9e2c4b4 into mui:master Mar 11, 2022
@m4theushw m4theushw deleted the speed-test-unit branch March 11, 2022 00:35
@m4theushw
Copy link
Member Author

Since we run unit tests in real browsers, is there any value in running same tests in jsdom?

@cherniavskii Personally I like them. The unit tests are easier to debug, which can be done inside VSCode. To debug the Karma tests I have to open a browser window and use Chrome DevTools.

@cherniavskii
Copy link
Member

The unit tests are easier to debug, which can be done inside VSCode.

We can keep them for development, but ignore in CI

@m4theushw
Copy link
Member Author

m4theushw commented Mar 11, 2022

We can keep them for development, but ignore in CI

@cherniavskii The purpose of the CI is to ensure that all tests will pass in a controlled environment. If we disable unit tests in the pipeline I guess we'll stop caring about them. This may cause that when developing a feature and we want to debug some piece of code we discover that the entire suit fails with JSDOM. Also, some users don't use a real browser to test, e.g. #2945

@cherniavskii
Copy link
Member

@m4theushw Good point

m4theushw added a commit to m4theushw/mui-x that referenced this pull request Mar 12, 2022
commit 4dacc49
Author: Matheus Wichman <matheushw@outlook.com>
Date:   Fri Mar 11 17:01:25 2022 -0300

    yarn docs:api

commit 7ca90ca
Author: Matheus Wichman <matheushw@outlook.com>
Date:   Fri Mar 11 16:34:31 2022 -0300

    Make reason optional

commit 835c66d
Author: Matheus Wichman <matheushw@outlook.com>
Date:   Fri Mar 11 16:24:26 2022 -0300

    Remove originalRow

commit ee0b648
Author: Matheus Wichman <matheushw@outlook.com>
Date:   Fri Mar 11 16:09:29 2022 -0300

    cellToMoveFocus -> cellToFocusAfter

commit 17f3bab
Author: Matheus Wichman <matheushw@outlook.com>
Date:   Fri Mar 11 16:07:20 2022 -0300

    Fix export bug with CSB

commit d3167e0
Author: Matheus Wichman <matheushw@outlook.com>
Date:   Fri Mar 11 16:07:02 2022 -0300

    Type value with any as default

commit 1b398f6
Merge: 3540d57 949b40a
Author: Matheus Wichman <matheushw@outlook.com>
Date:   Fri Mar 11 15:55:01 2022 -0300

    Merge branch 'master' into new-editing-api

commit 949b40a
Author: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Date:   Fri Mar 11 13:57:16 2022 -0300

    Bump actions/checkout action to v3 (mui#4107)

    Co-authored-by: Renovate Bot <bot@renovateapp.com>

commit 50f084d
Author: Andrew Cherniavskii <andrew@mui.com>
Date:   Fri Mar 11 15:53:18 2022 +0100

    [DataGrid] Fix error overlay not visible when `autoHeight` is enabled (mui#4110)

commit 3d85cc0
Author: Alexandre Fauquette <45398769+alexfauquette@users.noreply.github.com>
Date:   Fri Mar 11 15:03:20 2022 +0100

    [DataGrid] Fix white blank when scrolling (mui#4158)

commit 19fbfa5
Author: PBM <pbmalecki@wp.pl>
Date:   Fri Mar 11 11:47:42 2022 +0100

    [l10n] add missing plPL translations (mui#4153)

commit 8b2cd47
Author: Alexandre Fauquette <45398769+alexfauquette@users.noreply.github.com>
Date:   Fri Mar 11 10:10:12 2022 +0100

    [docs] Clean ValidateRowModelControlGrid demo (mui#4073)

commit 9e2c4b4
Author: Matheus Wichman <matheushw@outlook.com>
Date:   Thu Mar 10 21:35:46 2022 -0300

    [test] Mock `getComputedStyle` to speed up unit tests (mui#4142)

commit 1922f48
Author: Matheus Wichman <matheushw@outlook.com>
Date:   Thu Mar 10 14:35:09 2022 -0300

    [test] Upgrade CircleCI convenience image (mui#4143)

commit f04331c
Author: Flavien DELANGLE <flaviendelangle@gmail.com>
Date:   Thu Mar 10 17:38:18 2022 +0100

    v5.6.1 (mui#4141)

commit 3a507bf
Author: Andrew Cherniavskii <andrew@mui.com>
Date:   Thu Mar 10 16:05:29 2022 +0100

    [core] Upgrade `@mui/monorepo` (mui#4149)

commit 37766c8
Author: Matheus Wichman <matheushw@outlook.com>
Date:   Thu Mar 10 11:29:32 2022 -0300

    [DataGrid] Rename API method (mui#4148)

commit 1ac64ba
Author: Matheus Wichman <matheushw@outlook.com>
Date:   Wed Mar 9 15:12:56 2022 -0300

    [DataGrid] Add support for margin between rows (mui#3848)

commit 5551732
Author: Matheus Wichman <matheushw@outlook.com>
Date:   Wed Mar 9 09:38:18 2022 -0300

    [test] Make focus state out-of-sync warning opt-in (mui#4129)

commit 318f6cc
Author: Flavien DELANGLE <flaviendelangle@gmail.com>
Date:   Wed Mar 9 13:20:16 2022 +0100

    [DataGridPro] Re-export the components removed by mistake during bundle split (mui#4134)

commit c886092
Author: Flavien DELANGLE <flaviendelangle@gmail.com>
Date:   Wed Mar 9 13:19:52 2022 +0100

    [core] Prepare the api build scripts for multi packages support (mui#4111)

commit d7daee6
Author: Danail Hadjiatanasov <hadjiatanasov@gmail.com>
Date:   Wed Mar 9 11:49:38 2022 +0100

    [DataGrid] Display column's filter icon if a filter is applied (mui#4120)

commit 5e55a35
Author: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Date:   Wed Mar 9 11:41:50 2022 +0100

    Bump MUI Core (mui#4095)

    Co-authored-by: Renovate Bot <bot@renovateapp.com>

commit b8ed7a0
Author: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Date:   Wed Mar 9 11:41:43 2022 +0100

    Bump css-loader to ^6.7.1 (mui#4102)

    Co-authored-by: Renovate Bot <bot@renovateapp.com>

commit 3b34495
Author: Siriwat K <siriwatkunaporn@gmail.com>
Date:   Wed Mar 9 17:25:50 2022 +0700

    [docs] Fix links to prevent duplicate search result (mui#4130)

commit c80a645
Author: Andrew Cherniavskii <andrew@mui.com>
Date:   Wed Mar 9 10:50:15 2022 +0100

    [DataGrid] Fix extending built-in column types (mui#4114)

    Co-authored-by: Matheus Wichman <matheushw@outlook.com>

commit deb6615
Author: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Date:   Tue Mar 8 19:33:17 2022 -0300

    Bump typescript to ^4.6.2 (mui#4104)

    Co-authored-by: Renovate Bot <bot@renovateapp.com>

commit 1e8de15
Author: Matheus Wichman <matheushw@outlook.com>
Date:   Tue Mar 8 11:12:20 2022 -0300

    [core] Initialize remaining states before feature hooks (mui#4036)

commit fb4fc20
Author: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Date:   Tue Mar 8 13:30:33 2022 +0100

    Bump cpy-cli to ^4.0.0 (mui#4108)

    Co-authored-by: Renovate Bot <bot@renovateapp.com>

commit b0250ee
Author: Siriwat K <siriwatkunaporn@gmail.com>
Date:   Tue Mar 8 18:50:12 2022 +0700

    [docs] Use regex instead of specific url in e2e-website-tests (mui#4121)

commit ff35a77
Author: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Date:   Tue Mar 8 11:21:30 2022 +0100

    Bump react-router-dom to ^6.2.2 (mui#4100)

    Co-authored-by: Renovate Bot <bot@renovateapp.com>

commit 97e51fc
Author: Siriwat K <siriwatkunaporn@gmail.com>
Date:   Tue Mar 8 16:01:58 2022 +0700

    [docs] Neglect e2e tests related to search (mui#4118)

commit abd9bcf
Author: Olivier Tassinari <olivier.tassinari@gmail.com>
Date:   Mon Mar 7 19:56:46 2022 +0100

    [core] Make is clearer this is only for questions (mui#4082)

commit 2d1c394
Author: Vishal <32702012+patilvishal755@users.noreply.github.com>
Date:   Mon Mar 7 21:35:09 2022 +0530

    [docs] Fix outdated links to localeTextConstants.ts (mui#4080)

    Co-authored-by: Olivier Tassinari <olivier.tassinari@gmail.com>

commit 0be075d
Author: Flavien DELANGLE <flaviendelangle@gmail.com>
Date:   Mon Mar 7 15:23:24 2022 +0100

    [DataGrid] Do not loop through rows to compute top level rows count wen the tree is flat (mui#4081)
@oliviertassinari
Copy link
Member

is there any value in running same tests in jsdom?

For me, the value is about speed of iteration.
yarn t name-of-your-file and your test is up and running in no time. The data grid is the only component we have that heavily rely on layout, so for which jsdom can't test a lot of things.


As an important note, window.getComputedStyle is used by testing library for a11y assertions: https://github.com/testing-library/dom-testing-library/blob/0226aea74e873ba96dae414edc533c33b1e51867/src/role-helpers.js#L25. It goes beyond the use in the data grid. The speed up might come from there (if it's the same as testing-library/dom-testing-library#390).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants