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] testing-library patterns for playwright tests #25860

Merged
merged 5 commits into from
May 1, 2021

Conversation

eps1lon
Copy link
Member

@eps1lon eps1lon commented Apr 21, 2021

Implements some testing-library patterns in playwright in response to mui/mui-x#1443 (review).

One of the weak spots of our e2e tests using playwright is that they're using playwright API for querying and asserting. With this API the smallest matchers/queries are most of the time the less expressive ones. Expressive assertions need to provide information when they fail. Oftentimes developers only think about the happy path when writing tests which is not the primary concern of tests.

Example TrapFocus:
The playwright test for TrapFocus now mirrors its original mocha test.
https://github.com/eps1lon/material-ui/blob/0d400978012444913c9ba3d2f9951f4065b4bbe3/test/e2e/index.test.ts#L80-L99

-userEvent.tab();
+await page.keyboard.press('Tab');

-expect(screen.getByText('x')).toHaveFocus();
+ await expect(screen.getByText('x')).toHaveFocus();

Especially the assertion is now exactly the same. The required await is a language problem with JavaScript that we can't do anything about for now.

If we agree on the general direction I'll start working on feature parity with our mocha test so that people can potentially just copy&past mocha tests to playwright with minimal usage of playwright API.

Custom implementation over playwright-testing-library (PTL):

  1. PTL's API is still to verbose for my liking
  2. I don't quite follow their implementation.
    It seems to be tailored towards users that don't have control over the rendered page.
    This makes sense if you're testing a real page but we have dedicated fixtures in which we can inject @testing-library/dom easily.
  3. No assertion library
    The setup for the assertion library can be re-used for queries so PTL wouldn't solve any problem that we can't solve trivially.

I might just put this code into a dedicated package but first I'd like to get some core member feedback and then sync with PTL members in case my approach is flawed.

/cc @mui-org/maintainers @mui-org/x

@eps1lon eps1lon added the test label Apr 21, 2021
@mui-pr-bot
Copy link

mui-pr-bot commented Apr 21, 2021

No bundle size changes

Generated by 🚫 dangerJS against e9a34da

await page.keyboard.press('ArrowLeft');
expect(await page.evaluate(() => document.activeElement?.getAttribute('value'))).to.equal(
await expect(page.evaluateHandle(() => document.activeElement)).to.toHaveAttribute(
Copy link
Member Author

Choose a reason for hiding this comment

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

evaluateHandle is too powerful for regular usage I think. But it mirrors the expect(document.activeElement).to.have.attribute quite nicely. The alternative would be a custom API to get the focused element and neither playwright nor testing-library has one (since its implementation is trivial) so I'm hesitant to add one.
Let's just hope people don't abuse evaluateHandle more than they abuse document.querySelector.

@eps1lon eps1lon marked this pull request as ready for review April 21, 2021 08:47
Copy link
Member

@oliviertassinari oliviertassinari left a comment

Choose a reason for hiding this comment

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

It looks like a good step forward

@DanailH I got a conflict on the ports used trying to run the tests in this branch. What do you think of using different ports as we do for the documentation (3000 vs. 3001)?

@@ -1,5 +1,6 @@
import { expect } from 'chai';
import * as playwright from 'playwright';
import '../utils/initPlaywrightMatchers';
Copy link
Member

Choose a reason for hiding this comment

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

Did you consider?

Suggested change
import '../utils/initPlaywrightMatchers';
import 'test/utils/initPlaywrightMatchers';

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 use relative imports for in-module imports. For example, in core we import '../Button' not '@material-ui/core/Button'.

Copy link
Member

Choose a reason for hiding this comment

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

Regarding the why. Is it accurate that we only use in-module import in the core because we have to publish a package on npm? (that it would break if we didn't)

Copy link
Member Author

Choose a reason for hiding this comment

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

Doesn't matter if it's published on npm or not. Packages exist in node as well and imports are standardized in ES as well now. Using this absolute is just another thing one (computer or human) as to lookup and it's generally just not that important. Let's concentrate on the meat of the PR not subjective nits.

Copy link
Member

Choose a reason for hiding this comment

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

Agree, it's not directly relevant to the changes 👍 .

I guess more a general discussion for the future, not very relevant for this one-time instance.
So we could use absolute imports everywhere? I didn't know that, ok. Would it work if there are two times the package in the bundle with different versions? Will we risk importing from the wrong version of the package?

I'm asking because relative imports seem to make refactoring harder (need to change the import path if we move the folder) as well as seems to make the location resolution harder compared to absolute (developers have to know where the file is located and where the destination is, relative to it).

@eps1lon
Copy link
Member Author

eps1lon commented Apr 30, 2021

Going to limit this to the most common queries. For matchers we can add them as we go. The initial implementation can be used as a pattern when porting other matchers.

@eps1lon eps1lon marked this pull request as ready for review April 30, 2021 10:12
Comment on lines +68 to +72
getByLabelText: (...inputArgs) => {
return page.evaluateHandle(
(args) => window.DomTestingLibrary.getByLabelText(document.body, ...args),
inputArgs,
);
Copy link
Member Author

Choose a reason for hiding this comment

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

Imagine that code without rest parameters 😄

Suggested change
getByLabelText: (...inputArgs) => {
return page.evaluateHandle(
(args) => window.DomTestingLibrary.getByLabelText(document.body, ...args),
inputArgs,
);
getByLabelText: () => {
return page.evaluateHandle(
(args) => window.DomTestingLibrary.getByLabelText.apply(undefined, [document.body].concat(args)),
Array.prototype.slice.call(arguments),
);

@eps1lon eps1lon merged commit 327357c into mui:next May 1, 2021
@eps1lon eps1lon deleted the test/playwright-testing-library branch May 1, 2021 08:43
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.

None yet

3 participants