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

[core] Simplify testing architecture #6043

Merged
merged 16 commits into from
Sep 29, 2022

Conversation

flaviendelangle
Copy link
Member

@flaviendelangle flaviendelangle commented Sep 6, 2022

  • Use the @mui/monorepo setupJSDOM file to avoid duplicating code (equivalent of our current setup.js file which is badly named by the way).
    This has a side effect. The core adds the url: 'http://localhost' property to its JSDOM instance, which breaks our print tests because printWindow.contentDocument.body is now null (don't know why).
    If we don't find a fix, I can either copy / paste the setupJSDOM file to remove the url property or make it optional on the core, or skip the printing test when using JSDOM (what I currently did).

  • Use the same file naming strategy as in the core

  • Set the license key before each test to prepare [license] Add new license status 'Out of scope' #5260 (comment)

  • Avoid mocha hook duplication (for example we had a sinon.restore on both setup.js for test:unit and on karma.test.js for test.karma.

@flaviendelangle flaviendelangle self-assigned this Sep 6, 2022
@flaviendelangle flaviendelangle added the core Infrastructure work going on behind the scenes label Sep 6, 2022
@mui-bot
Copy link

mui-bot commented Sep 6, 2022

These are the results for the performance tests:

Test case Unit Min Max Median Mean σ
Filter 100k rows ms 490.8 1,076.9 503.4 739.84 226.251
Sort 100k rows ms 611.4 1,279.1 1,279.1 1,010 244.239
Select 100k rows ms 226.5 353.9 290.6 299.68 46.271
Deselect 100k rows ms 132.6 278.1 229.2 225.18 51.64

Generated by 🚫 dangerJS against 949b81a

package.json Show resolved Hide resolved
@@ -1,3 +1,4 @@
require('@babel/register')({
extensions: ['.js', '.ts', '.tsx'],
ignore: [/node_modules\/(?!@mui\/monorepo)/],
Copy link
Member Author

Choose a reason for hiding this comment

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

We had two babel files with almost the same config.
I removed the one in the old setup.js file and used this one everywhere.


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

Choose a reason for hiding this comment

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

Copied from the old createDOM.js file

@flaviendelangle flaviendelangle changed the base branch from master to next September 9, 2022 09:35
@flaviendelangle flaviendelangle marked this pull request as ready for review September 12, 2022 09:07
@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Sep 12, 2022
@github-actions

This comment was marked as outdated.

1 similar comment
@github-actions

This comment was marked as outdated.

@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Sep 13, 2022
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.

Looks great!

test/utils/setupKarma.js Outdated Show resolved Hide resolved
@github-actions
Copy link

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Sep 27, 2022
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Sep 29, 2022
@oliviertassinari
Copy link
Member

Nice cleanup :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Infrastructure work going on behind the scenes test
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants