Performance concern with jsdom and getByRole

[oliviertassinari]

• DOM Testing Library version: 6.8.1
• node version: v12.6.0

I apologize in advance for not following the issue template; I'm not sure it serves performance discussion well.

We are in the progress of migrating the test suite of Material-UI from enzyme shallow + mount API to testing-library. However, we have hit a roadblock (we should consider what we do with this block, do we force our way in, move around it, etc?). While the test suite takes about 14s to run on v3, it now takes about 70s (on my machine). It's a x5 increase. Diving more into the issue, I could identify this problem:

Once we aggregate all the data, we realize that we indeed spend most of our time in this function:

So it seems that the numerous calls to window.getComputedStyle() are using +65% of the available CPU resources. If we run the same tests in a chrome headless, the performance are much better, about 20s instead of 70s.

So my question is, do we have a viable workaround? In the case of Material-UI, we have migrated about 28% of our tests. If I extrapolate, once we hit 100%, our unit tests will take about 4 minutes (to be compared with ~14s in v3). Should we do something about it? If yes, what options do we have? Thank you!

[kentcdodds]

Cc @eps1lon... This is pretty bad. I'm starting to think we should switch this from an opt out to an opt in...

[eps1lon]

Honestly a 5x hit is somewhat ok for me considering it's an increase in confidence. I don't really buy in into perf for tests especially if we are still in the 1 minute ballpark. We should sync first if we prioritize fast or safe tests. I at least won't put much work into making tests fast. I outlined workarounds and I'm happy to assist with the work that can be done.

[eps1lon]

Machine time is for me personally one of the least important concerns. So while I think this shouldn't matter I'm a consequentialist at the end of the day and if nobody tests a11y because of X then I still want to improve X.

I can't make any guarantees about the gains though. It'll always be slower than whatever your testing approach was before. If you are ok with low-quality¹, inaccessible software then you should switch back to enzyme.

¹ we know very little about software engineering but one of the things that have been shown in research again and again is that testing improves software quality.

Edit:

Also note that these times are very misleading. We didn't "just" migrate our tests. The migration gave us a huge confidence boost and help uncover bugs. We also increased the number of tests (I guess the v3.x branch is used) from 1784 to 2200

[oliviertassinari]

I'm eager to hear @kentcdodds perspective on the fast vs exhaustiveness tradeoff as he's passionate about the topic.

My intuition on the topic regarding the incentives 1. the slower the tests, the less likely a developer will write new tests. 2. changes that impact a large portion of the codebase might require to run all of them. A longer feedback loop discourages to undergo such efforts.

I don't think that the issue is with testing-library per se, nor that it challenges the migration to it. I think that it covers how we query elements. Which raises the question, does getByRole worth the ~x10 increase in test duration? Should we consider/offer another tradeoff?

[eps1lon]

Which raises the question, does getByRole worth the x15 increase in test duration?

Where did you get this number from?

Edit:
Also I don't think this is a good faith argument. Sure you can toss out these scary looking "A wooping 10x increase". But we're still talking about a 60s runtime on a dev machine. Who cares about this time? I'm not interested in making the life of highly privileged developers easier. I want them to help write good software.

Edit2:
Did a quick measurement skipping isAccessbile in our test suite. It's 18s vs 52s which is a 3x increase. Again: No guarentees how faster this can be (probably requires jsdom work) but it's nowhere near as bad as Olivier communicates.

[oliviertassinari]

@eps1lon Here are different estimation approaches. But these are rough estimates, It seems to be around the order of magnitude:

1. Tests took ~14s before, ~28% of the tests migrate, tests take ~70s now, extrapolate. 70/14/0.28 = x17 but v3 tests are not the same. The figure is likely smaller.
2. Run the menu tests with and without getByRole, measure the time duration diff. From around 700ms per test to under the mocha slow test warning (75ms): ~x10
3. 68% of the time is spend in computing styles: 1/.68/.28 = x5 increase, but with garage collection, this figure might be higher.
4. Did a quick measurement skipping isAccessbile in our test suite. It's 18s vs 52s which is a 3x increase.: 52/18/.28 = about a x10 increase.

[eps1lon]

Tests took ~14s before, ~28% of the tests migrate, tests take ~70s now, extrapolate.

Ok I won't engage with you in this discussion anymore. There's no good faith attempt from you here since we increased the test and fixed bugs.

[kentcdodds]

I don't think there's bad faith at all. @oliviertassinari may be mistaken in calculations, but it's not bad faith. Just want to clear that up and keep our conversation friendly 🤗

I'm eager to hear @kentcdodds perspective on the fast vs exhaustiveness tradeoff as he's passionate about the topic.

I personally favor confidence over speed by a long shot, but there's a certain point where tests are slow enough that they necessitate making the trade-off of some confidence in favor of speed otherwise people will write and run fewer tests which leaves us worse off.

I think the good compromise here would be to make the isAccessible call configurable globally. Shouldn't take a lot of work to do that. It would involve adding the configuration default to right around here:

dom-testing-library/src/config.js

Line 5 in 79661e1

testIdAttribute: 'data-testid',

and then updating the default assignment to use that config here:

dom-testing-library/src/queries/role.js

Line 11 in 79661e1

{exact = true, collapseWhitespace, hidden = false, trim, normalizer} = {},

Then a single test for it would be sufficient. I think we should leave it at its current default, but making it configurable should help people when they decide they'd like to trade off their confidence in favor of speed (which I think is a reasonable trade off for this situation).

[eps1lon]

Ok I got a plan that should help if there are multiple items returned (e.g. menuitem):

Understandable? 😄

[oliviertassinari]

Update: mui/material-ui#18015 has speeded up tests by a large margin (from 70 seconds to 47 seconds on my machine).

[oliviertassinari]

I have kept digging a bit. Here is how compares Material-UI tests between node and chrome (karma):

• node bootstrap time: 10s
• node run time: 47s
• chrome bootstrap time: 33s
• chrome run time: 21s

This suggests that there is still the potential to save a good 20s. As it turns out, If I replace window.getComputedStyle(element) with element.style. The node tests run in 24s.

So my question would be, would it worth to provide an opt-in option to only support inline style (with .style) and, as a consequence, be blind to the CSS people might inject in the page in order to speedup JSDOM test?

To be honest, I'm not sure it would. I would be eager to continue without any change for the next few months, to gain more knowledge of each side's value for this tradeoff.

As of now, all the +2,000 tests of Material-UI are green without the usage of window.getComputedStyle(). If isInaccessible() always return true, we have 10 failing tests ❌.

[kentcdodds]

I don't think it would be wise to just rely on .style. I think we should do all of it or none of it.

[eps1lon]

We are now down to 30s runtime with jsdom and there's another change that's merged but unreleased in jsdom that will cut another 5s (jsdom/jsdom#2706).

We'll experiment a bit with changing the default between local development and CI (CI being a better candidate for expensive tests) while still respecting explicit options.

For example getByRole('option') would mean you're sure there's only the one option (but CI verifies this), getByRole('option', { hidden: false }) means you know there are other options but they aren't accessible and getByRole('option', { hidden: true }) means you know it's inaccessible but want to assert something anyway.

Another compromise that's growing on me is adding another option that solely checks for something being hidden from assistive technology (i.e. contained within aria-hidden). The rationale being that if you make assertions about something being visually¹ hidden then at least your component fails for everybody not just folks relying on assistive technology.

To summarize: I'd consider this resolved in so far that it isn't that much worse than other queries.

¹ Only display and visibility are considered. The isInaccessible cannot verify if sighted users can use your component (e.g. skip-links in their visually-hidden state).

[oliviertassinari]

@eps1lon Well done! It sounds like we can push the testing library migration to its full extent without having to worry about performance ✌️.

[oliviertassinari]

Anything preventing to close the issue?

(MUI issue was solved at 💯)

[kentcdodds]

I'm fine with closing this. Thanks everyone!