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] Skip JSDOM in style related conformance tests #24668

Merged
merged 2 commits into from
Jan 28, 2021

Conversation

mnajdova
Copy link
Member

@mnajdova mnajdova requested a review from eps1lon January 28, 2021 10:59
@mui-pr-bot
Copy link

mui-pr-bot commented Jan 28, 2021

No bundle size changes

Generated by 🚫 dangerJS against e11859b

@oliviertassinari
Copy link
Member

oliviertassinari commented Jan 28, 2021

Why do we need this change? yarn t X only runs with JSDOM, the more tests we skip between environments the worse the DX is for maintaining these tests is (longer feedback loop).

@eps1lon
Copy link
Member

eps1lon commented Jan 28, 2021

Why do we need this change?

Testing computed styles is unreliable in JSDOM. See #24661 (comment) and implementation notes of toHaveComputedStyles.

@mnajdova
Copy link
Member Author

After applying 9824e08 #24661 is no longer blocked by this change, but I still think we should do this, as debugging issues like this in the future will be hard

@eps1lon
Copy link
Member

eps1lon commented Jan 28, 2021

Also: I would hope that these are not tests you need to work with very often. That would kind of indicate that they shouldn't be part of a conformance test suite. I can improve documentation for test:karma if it's unclear how to get a fast feedback loop. But considering I've been explaining for the longest time how --grep and .only can be used and this advise being ignored again and again, I don't think this will be heard.

@oliviertassinari What you want can only be achieved by having fewer components. But that's not what you've been pushing for. You can either implement more components or have a smaller test suite. Having both results in a lower test confidence i.e. lower quality components. So if you want more components and keep the test confidence you have to adjust how you work with the test suite. I haven't seen that you've been willing to do that. It may be just an API issue but if you're just ignoring me I can't help you. I need explicit feedback to be able to improve our test workflow. You just coding away and complaining every time that "tests are slow" does not help anyone. Not me in speeding up the tests and not Marija who's implementing the work you've envisioned.

@mnajdova mnajdova merged commit a34457b into mui:next Jan 28, 2021
@oliviertassinari
Copy link
Member

oliviertassinari commented Jan 28, 2021

Ok, at least we are aware of the tradeoff, we can always course correct in the future if it becomes an issue :).

I would hope that these are not tests you need to work with very often

Yeah, it's very possible, I think that it's only now, because we work on the migration that we spend time on them. I have been helping with the migration of 10 components to emotion last weekend, this test has been a friction point, even for the contributors.

I need explicit feedback to be able to improve our test workflow.

@eps1lon Sure! From a high-level perspective, I think that we can leverage the tests to solve two problems:

  • Create constraints in place so we don't lose knowledge. One can argue that with enough time, we will be able to put the piece together if we introduce regressions but I believe we can easily degrade the solution if we don't have all the context (the harder a problem is, the more relevant this is).
  • Allow the maintainers to move faster, either by allowing a path that isn't as costly as the "regression -> release -> user feedback -> fix" cycle or by giving us more confidence when refactoring.

Some might see other benefits like not introducing regressions for developers using the library or working in TDD improving the quality of the implementation. I don't really buy into these arguments in our context (e.g. we don't need 99.99% availability, and developers can revert version, nor all upgrade at once when we release) but I can respect them.

Over the last few weeks, I have personally experienced the following pain:

  1. Slowness in the CI, we have gone above the 15 minutes CI duration time, when there is no starvation. Getting it below 10 minutes consistently would be a big win.
  2. Flakiness in the CI, having to rerun builds is context shifting

I would love to hear Marija's frustration too. Maybe we should allocate a $1,000/month budget to increase the hardware resources available in the CI cc @mbrookes.

natac13 pushed a commit to natac13/material-ui that referenced this pull request Jan 30, 2021
@oliviertassinari oliviertassinari changed the title [tests] Skip JSDOM in style related conformance tests [test] Skip JSDOM in style related conformance tests Feb 10, 2021
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