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] Replace enzyme in describeConformance #42447

Merged

Conversation

DiegoAndai
Copy link
Member

@DiegoAndai DiegoAndai commented May 29, 2024

Part of #42454

This PR removes createMount from the describeConformance testing utility. Some notes:

  • I strongly encourage reviewers to review commits separately. I divided commits by test, so it will be easier to understand each change.
  • I provided an overview of each test change.
  • For Joy UI tests that failed, I simply skipped them.

@DiegoAndai DiegoAndai added test core Infrastructure work going on behind the scenes labels May 29, 2024
@DiegoAndai DiegoAndai self-assigned this May 29, 2024
@mui-bot
Copy link

mui-bot commented May 29, 2024

Netlify deploy preview

https://deploy-preview-42447--material-ui.netlify.app/

Bundle size report

Details of bundle changes (Toolpad)
Details of bundle changes

Generated by 🚫 dangerJS against 3a6ae48

@LukasTy LukasTy changed the title [code] Replace enzyme in describeConformance [core] Replace enzyme in describeConformance May 30, 2024
@DiegoAndai DiegoAndai force-pushed the remove-enzyme-from-describe-conformance-2 branch 7 times, most recently from b711d40 to c329f67 Compare May 30, 2024 17:49
@DiegoAndai DiegoAndai mentioned this pull request May 30, 2024
4 tasks
@DiegoAndai DiegoAndai force-pushed the remove-enzyme-from-describe-conformance-2 branch from c486d38 to 81f9574 Compare May 30, 2024 19:40

expect(root.props()).to.have.property(testProp, value);
expect(getByTestId(testId)).to.have.attribute(testProp, value);
Copy link
Member Author

Choose a reason for hiding this comment

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

Here, we lose the aspect of testing that the inherited component is being used and only maintain that the props are spread into whatever the component considers its root.

This follows RTL's principle of not testing implementation details. There might be a way for us to test that the inherited component is the one used, but I think it would be better to test that on each component's tests, by testing the expected functionality, classes, or whatever the inherited component provides.

Copy link
Member

Choose a reason for hiding this comment

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

AFAIK this is mostly used by the doc gen to add a message about additional props available on a component. I'm strongly in favor of removing this message and listing all available props on each component, so it's not necessary to jump between the pages to learn about available props. Afterwards, this test may be unnecessary.

Not in this PR, of course :)

@@ -269,6 +268,25 @@ function testSlotPropsProp(
});
}

function testClassName(element: React.ReactElement, getOptions: () => ConformanceOptions) {
Copy link
Member Author

@DiegoAndai DiegoAndai May 30, 2024

Choose a reason for hiding this comment

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

This was required as Base UI's describeConformance accepts async render functions. We could support those in the basic describeConformance test, but I wanted to keep the changes minimal, especially considering this base code is stale. Let me know if making the original testClassName async would be better.

Copy link
Member

Choose a reason for hiding this comment

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

Async render functions will be required when Material UI uses Floating UI. Certain interactions require flushing the microtask queue, which is an async operation. This is not strictly required now, so I agree it's ok to minimize the number of changes in this PR.

@DiegoAndai DiegoAndai marked this pull request as ready for review May 30, 2024 19:55
@DiegoAndai DiegoAndai requested review from Janpot and michaldudak and removed request for Janpot May 30, 2024 19:55
@cherniavskii cherniavskii mentioned this pull request Jun 5, 2024
1 task
@aarongarciah
Copy link
Member

aarongarciah commented Jun 17, 2024

@DiegoAndai just ran the describeConformance tests locally with react@18.3.1 and @types/react@18.3.3 (both are latest). Only one test was failing:

Screenshot 2024-06-17 at 11 10 13

This is solved via 117cf8a and now all describeConformance tests pass locally using react@18.3.1 and @types/react@18.3.3.

There are several failures when running all unit tests with react@18.3.1, but I think those should probably be addressed in the corresponding PR upgrading React: #42047

@aarongarciah aarongarciah force-pushed the remove-enzyme-from-describe-conformance-2 branch from ac2582a to 117cf8a Compare June 17, 2024 09:23
@aarongarciah
Copy link
Member

I just rebased to solve CI issues.

@aarongarciah
Copy link
Member

aarongarciah commented Jun 17, 2024

@DiegoAndai after updating all React related deps in all the repo packages, describeConformanceUnstyled from Base UI fails:

Screenshot 2024-06-17 at 12 19 32 Screenshot 2024-06-17 at 12 20 13

After fixing the first error (just an import path update), we'll need to deal with other deprecations. I think we should take care of this in #42047.

@DiegoAndai
Copy link
Member Author

Thanks for picking this up @aarongarciah

There are several failures when running all unit tests with react@18.3.1, but I think those should probably be addressed in the corresponding PR upgrading React: #42047

Yes, many things must be fixed, but I want to keep this PR scoped in describeConformance as MUI X relies on it. We can fix the other tests in a separate PR. This also includes describeConformanceUnstyled.

@LukasTy we're ready to merge this on our side, what about from the MUI X side?

@DiegoAndai DiegoAndai requested a review from LukasTy June 17, 2024 14:58
@cherniavskii
Copy link
Member

@LukasTy In mui/mui-x#13360 I've updated @mui/internal-test-utils to use the latest commit from this branch.
Feel free to use my branch to resolve issues related to @mui/internal-test-utils, or open a separate PR if that's more convenient.

@cherniavskii
Copy link
Member

@DiegoAndai Everything looks good on the MUI X side: mui/mui-x#13360
Let's wait for @LukasTy's approval though, because there might be something I'm not aware of 🙂

Copy link
Member

@LukasTy LukasTy left a comment

Choose a reason for hiding this comment

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

Thank you for all the effort on it. 🙏
The solution looks great.
I've updated the mui/mui-x#12295 PR with the latest package from this PR and made some cleanup changes for things that are no longer used like this in this PR. 😉

@aarongarciah
Copy link
Member

For some reason all tests passed in CI but 37 tests fail locally when running test:unit 😅 I'm looking into it.

@aarongarciah aarongarciah force-pushed the remove-enzyme-from-describe-conformance-2 branch from 117cf8a to 3a6ae48 Compare June 18, 2024 13:39
@Janpot
Copy link
Member

Janpot commented Jun 18, 2024

@aarongarciah Could be related to #42670.

@aarongarciah
Copy link
Member

After a rebase all tests pass locally. We had a regression causing tests to not run in CI but now that's fixed. More info https://mui-org.slack.com/archives/C042YB5RB3N/p1718610795073829

@aarongarciah
Copy link
Member

aarongarciah commented Jun 18, 2024

Could be related to #42670

Yes, just saw the fixes. Thank you @Janpot

@aarongarciah aarongarciah merged commit da7b8cf into mui:next Jun 18, 2024
22 checks passed
@DiegoAndai DiegoAndai deleted the remove-enzyme-from-describe-conformance-2 branch June 18, 2024 16:09
@DiegoAndai DiegoAndai added the React 19 support PRs required to support React 19 label Jun 21, 2024
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 React 19 support PRs required to support React 19 test
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants