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

Experimenting integration testing best practices with RTL and msw #27027

Closed
wants to merge 14 commits into from

Conversation

kevin940726
Copy link
Member

@kevin940726 kevin940726 commented Nov 17, 2020

Description

tdlr;

Use msw and RTL, to do integration testing based on best practices (avoid mocking as much as possible and only test public API rather than implementation details). It's a lot slower than unit tests, but faster than e2e tests. Hopefully we can have more tests like this so that we can speed up our tests when e2e is too slow and bloated.

Most tests in Gutenberg are unit tests (run by Jest on each component / utility function) and e2e tests (run by Jest and Puppeteer on a real browser environment). We have a big chunk of tests that should just be integration tests but we often instead write them as e2e tests, which are very powerful but also very very slow. This PR tries to lay down the foundation of integration testing in this repo and also encourage best testing practices.

To begin with, we have to first set goals of what we're trying to do in our tests.

Goals

  1. Use only public APIs in tests.
  2. Avoid mocking as much as possible but still keep tests consistent, move the mocks to the lowest level.
  3. Write tests the way the user would actually use it.
  4. Querying should be accessible, in other words, avoid using class or id selectors, use accessible role, text, or label.

With the goals outlined, we can come up with a better integration testing infastructure with the helps from some popular libraries.

React Testing Library (RTL)

RTL is already used in some of our unit tests. Introduced in #20428, it became the recommended way to write tests both in Gutenberg but also recommended in the official React doc. Using it in the integration testing forces us to write better tests which resembles the way the feature is actually used, satsifying both 1, 3, and 4 of the goals.

@testing-library/jest-dom

Using it with RTL gives us a set of custom matchers that matches the usage of RTL. Instead of writing this every time.

expect(node.textContent).toBe('TEXT');

We can now write this instead.

expect(node).toHaveTextContent('TEXT');

msw

Network layer mocking solution. Instead of mocking fetch directly in tests via jest.spyOn or jest.fn, we can now just define it globally once with mocked fixture data. Once setuped, any requests in the tests will directly get passed to the mock function, making mocking seamless.

Though we are focusing on integration testing in this PR, msw is not only useful in integration testing. In the future, we can also use msw for some of our e2e tests, storybook, or even during regular development. Enable us to create a unified experience of network mocking among the project.

How has this been tested?

Run the test with the following command

npm run test-unit -- packages/edit-widgets/src/test/index.test.js

Not sure why it won't terminate after tests pass though, but it's already happening in master and in every test.

Types of changes

New feature

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.
  • I've updated all React Native files affected by any refactorings/renamings in this PR.

@kevin940726 kevin940726 added [Type] Enhancement A suggestion for improvement. [Type] Automated Testing Testing infrastructure changes impacting the execution of end-to-end (E2E) and/or unit tests. labels Nov 17, 2020
@@ -36,7 +36,7 @@ export default function BlockParentSelector() {
parentBlockType: _parentBlockType,
firstParentClientId: _firstParentClientId,
shouldHide: ! hasBlockSupport(
parentBlockType,
_parentBlockType,
Copy link
Member Author

Choose a reason for hiding this comment

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

I discovered a bug when writing these tests!

@github-actions
Copy link

github-actions bot commented Nov 17, 2020

Size Change: +35 B (0%)

Total Size: 1.2 MB

Filename Size Change
build/block-editor/index.js 133 kB -5 B (0%)
build/edit-widgets/index.js 26.4 kB +40 B (0%)
ℹ️ View Unchanged
Filename Size Change
build/a11y/index.js 1.14 kB 0 B
build/annotations/index.js 3.8 kB 0 B
build/api-fetch/index.js 3.42 kB 0 B
build/autop/index.js 2.83 kB 0 B
build/blob/index.js 665 B 0 B
build/block-directory/index.js 8.72 kB 0 B
build/block-directory/style-rtl.css 943 B 0 B
build/block-directory/style.css 942 B 0 B
build/block-editor/style-rtl.css 11.3 kB 0 B
build/block-editor/style.css 11.3 kB 0 B
build/block-library/editor-rtl.css 8.96 kB 0 B
build/block-library/editor.css 8.96 kB 0 B
build/block-library/index.js 148 kB 0 B
build/block-library/style-rtl.css 8.23 kB 0 B
build/block-library/style.css 8.23 kB 0 B
build/block-library/theme-rtl.css 792 B 0 B
build/block-library/theme.css 793 B 0 B
build/block-serialization-default-parser/index.js 1.87 kB 0 B
build/block-serialization-spec-parser/index.js 3.06 kB 0 B
build/blocks/index.js 48.1 kB 0 B
build/components/index.js 172 kB 0 B
build/components/style-rtl.css 15.3 kB 0 B
build/components/style.css 15.3 kB 0 B
build/compose/index.js 9.93 kB 0 B
build/core-data/index.js 14.8 kB 0 B
build/data-controls/index.js 827 B 0 B
build/data/index.js 9.71 kB 0 B
build/date/index.js 11.2 kB 0 B
build/deprecated/index.js 769 B 0 B
build/dom-ready/index.js 571 B 0 B
build/dom/index.js 4.92 kB 0 B
build/edit-navigation/index.js 11.2 kB 0 B
build/edit-navigation/style-rtl.css 881 B 0 B
build/edit-navigation/style.css 885 B 0 B
build/edit-post/index.js 306 kB 0 B
build/edit-post/style-rtl.css 6.45 kB 0 B
build/edit-post/style.css 6.44 kB 0 B
build/edit-site/index.js 23.5 kB 0 B
build/edit-site/style-rtl.css 3.86 kB 0 B
build/edit-site/style.css 3.86 kB 0 B
build/edit-widgets/style-rtl.css 3.13 kB 0 B
build/edit-widgets/style.css 3.13 kB 0 B
build/editor/editor-styles-rtl.css 476 B 0 B
build/editor/editor-styles.css 478 B 0 B
build/editor/index.js 43.3 kB 0 B
build/editor/style-rtl.css 3.85 kB 0 B
build/editor/style.css 3.85 kB 0 B
build/element/index.js 4.62 kB 0 B
build/escape-html/index.js 735 B 0 B
build/format-library/index.js 6.86 kB 0 B
build/format-library/style-rtl.css 547 B 0 B
build/format-library/style.css 548 B 0 B
build/hooks/index.js 2.16 kB 0 B
build/html-entities/index.js 623 B 0 B
build/i18n/index.js 3.57 kB 0 B
build/is-shallow-equal/index.js 698 B 0 B
build/keyboard-shortcuts/index.js 2.85 kB 0 B
build/keycodes/index.js 1.94 kB 0 B
build/list-reusable-blocks/index.js 3.1 kB 0 B
build/list-reusable-blocks/style-rtl.css 476 B 0 B
build/list-reusable-blocks/style.css 476 B 0 B
build/media-utils/index.js 5.31 kB 0 B
build/notices/index.js 1.82 kB 0 B
build/nux/index.js 3.42 kB 0 B
build/nux/style-rtl.css 671 B 0 B
build/nux/style.css 668 B 0 B
build/plugins/index.js 2.56 kB 0 B
build/primitives/index.js 1.43 kB 0 B
build/priority-queue/index.js 790 B 0 B
build/redux-routine/index.js 2.84 kB 0 B
build/reusable-blocks/index.js 3.07 kB 0 B
build/rich-text/index.js 13.4 kB 0 B
build/server-side-render/index.js 2.77 kB 0 B
build/shortcode/index.js 1.69 kB 0 B
build/token-list/index.js 1.27 kB 0 B
build/url/index.js 4.05 kB 0 B
build/viewport/index.js 1.86 kB 0 B
build/warning/index.js 1.14 kB 0 B
build/wordcount/index.js 1.22 kB 0 B

compressed-size-action

@kevin940726 kevin940726 force-pushed the try/edit-widgets-integration-testing branch from 0bb3168 to 053c4a4 Compare November 17, 2020 09:11
@talldan
Copy link
Contributor

talldan commented Nov 17, 2020

@kevin940726 Seeing this just reminded me of an earlier PR - #18855.

@kevin940726 kevin940726 force-pushed the try/edit-widgets-integration-testing branch from 0a3f916 to bd87ce8 Compare November 17, 2020 09:56
@@ -23,24 +23,32 @@ import { create as createLegacyWidget } from './blocks/legacy-widget';
import * as widgetArea from './blocks/widget-area';
import Layout from './components/layout';

let hasInitialized = false;
Copy link
Contributor

Choose a reason for hiding this comment

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

we should add some comment about what is this here for.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm, I'm not sure what to add here. As the code itself is pretty self-explanatory 🤔? I most likely have a blind spot here though.

Copy link
Contributor

@draganescu draganescu left a comment

Choose a reason for hiding this comment

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

Just a few minor nitpicks, but this reads and looks great. Tested locally and the tests work great! Good job!

Copy link
Member

@gziolo gziolo left a comment

Choose a reason for hiding this comment

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

Any idea where the change to the size of @wordpress/element bundle is coming from?
Screen Shot 2020-11-19 at 15 35 53

I'm really concerned about the performance implications of the proposed tests. Is there any way to make them significantly faster?

import './mocks/server';
import availableLegacyWidgets from './mocks/available-legacy-widgets';

// We need this since some tests run over 5 seconds threshold.
Copy link
Member

Choose a reason for hiding this comment

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

This is very concerning that this test requires a longer timeout which is close to what we use with e2e tests.

All existing unit tests pass in 45-ish seconds on my machine:

Test Suites: 430 passed, 430 total
Tests:       2 skipped, 4631 passed, 4633 total
Snapshots:   197 passed, 197 total
Time:        43.828s, estimated 72s

It means that 10 integration tests that run above the existing timeout would run longer than 4 633 unit tests.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, only the second test takes that long though. I think it might be an existing performance bug with our component but not a problem with the test itself. I'll try to do some more research to find out why it's so slow.

@kevin940726
Copy link
Member Author

Any idea where the change to the size of @wordpress/element bundle is coming from?

It should be the size of the package react-dom/test-utils. I'm not sure how it works with wp.element, but if it's imported using webpack then it should be tree-shakable.

Now that I look at it, we might need to move act into a sub package as well, like @wordpress/element/test-utils?

@noisysocks
Copy link
Member

This is really cool!

We have a big chunk of tests that should just be integration tests but we often instead write them as e2e tests, which are very powerful but also very very slow.

Do you have numbers on how long it takes an integration test to run vs an equivalent E2E test? When would/should one choose one over the other?

It's a new package created in this PR. It's essentially a fork of RTL, but running in the @wordpress/element context rather than the React context.

Is there an alternative to forking? How will we keep our fork up to date with upstream changes?

@kevin940726
Copy link
Member Author

@noisysocks Great questions!

Do you have numbers on how long it takes an integration test to run vs an equivalent E2E test?

I don't, for now 😅, since there aren't many e2e tests for the widgets screen either. But in theory, integration tests should almost always be faster than e2e tests. We're only testing a specific package using jsdom in contrast of spawning a whole docker instance, loading all the PHP/JS/HTML/CSS assets, and running a real (headless) browser environment via puppeteer. So, yeah, it should be significantly faster.

However, there's an integration test which is taking over 7s in this PR. I believe it should be a bug of our components though, no way it should take that long. Might have something to do with #26724, I'll do some more digging into this issue.

When would/should one choose one over the other?

Popularized by Kent C. Dodds in his article "Write tests. Not too many. Mostly integration.", IMO we should be doing integration tests wherever possible. Sometimes integration tests won't be enough (like if we want to test real browsers' behavior or do visual testing), then we'll forward it to the much slower e2e tests.

Is there an alternative to forking? How will we keep our fork up to date with upstream changes?

We might be able to do some Jest specific config to transform "react" call to "@wordpress/element" automatically (or maybe it has already been done!?). Then we don't have to maintain the fork and just let the package manager handles it. I'll give it a try later!

FWIW, most official testing libraries are essentially using the same way we do here though. See preact-testing-library for example.

@youknowriad
Copy link
Contributor

Doing integration tests with mocks doesn't sound compelling to me. The REST API can change and we'd not be catching bugs and e2e tests allow us to test way more (all the PHP code) which is very important for widgets and FSE (PHP unit tests are not enough).

So yeah e2e tests are slower than tests mocking APIs but I disagree with us doing more and more integration tests instead of e2e tests.

I'm not against experimenting with these but I'm concerned about both the false positives and the cost of maintenance.

As you can read, I have a very different perspective https://riad.blog/2020/07/21/deleting-tests-is-a-best-practice/

@youknowriad
Copy link
Contributor

Also we run Gutenberg e2e tests on Core for each release and it's a great way for us to validate that the integration in Core works just as well as in the plugin. We catch a number of missing backports by doing so.

@kevin940726 kevin940726 force-pushed the try/edit-widgets-integration-testing branch from 9123064 to 21ab6cd Compare November 20, 2020 07:59
@kevin940726
Copy link
Member Author

@youknowriad I mostly agree that e2e tests let us catch more bugs and resemble the most closely to the way users use the software. However, IMHO, doing integration tests is still useful in some ways:

  1. Faster, not only do the tests run faster but also the developer experience feels way faster. Being able to run them in jsdom enables us to run multiple tests in parallel. And also, because we don't have to spawn a headless browser environment, the initial startup time is much faster than e2e tests.
  2. The tests are easier to write/read. Writing e2e tests has to be done in an "asynchronously-way". We can't directly access the element node, we have to get its "ElementHandle", then access their property via .evaluate() or something similar. It's not that big of a problem, but doesn't it feel nice if we can just write expect(node.textContent).toBe('SOMETHING')? The Same thing goes with reading the source code of the tests, maybe it's just me, but it just feels more natural to have the assertions looks the same way as in the browser environment.
  3. Separation of concerns. Gutenberg is a mon-repo with multiple packages. Ideally, one should be able to use a package (and with its dependencies) without the others. Doing package-level integration testing let us test just that with minimal setup required. The tests could also be living documentation of the package, and much easier to reason about for first-time contributors. At least for me, I've been spending quite an amount of time trying to understand how each package works in isolation so that it's so much easier for me to discover bugs and trace code 😅 .
  4. I'm not familiar with the backend/PHP side of the plugin, it's much easier for me just care about the frontend sometimes. Being able to mock the network response with fixture data is a huge advantage to me. Though I do agree that we should avoid mocking as much as possible, hence it's also mentioned in the second goal above.
  5. Sometimes mocking is necessary/preferred. For instance, when testing a certain broken state that cannot be easily reproduced but we still need to support for backward-compatibility. We even have this setUpResponseMocking e2e test utility already. Instead of performing the same operations to create a post over and over again, I believe it's considered a best practice to test it step-by-step only once and use the mocked result for the rest of the tests.
  6. E2E tests are still very useful, the rule of thumb is that we should do e2e tests for features that are being used a lot or could easily break, and also do integration tests as complements to the existing e2e tests. If the REST API changed for some reason, our e2e tests can easily spot that, but we don't need to do that for every test. Instead, if most of the time the REST API won't change, then we should do more integration testing for all the benefits listed above.

If you think about it, the integration tests introduced here is kind of like e2e tests for the package rather than for the whole app 😂 .

@kevin940726
Copy link
Member Author

For @wordpress/testing-library, I ended up removing that package since seems like we don't need it yet. Right now it's just msw + RTL in the tests 🎉 .

@youknowriad
Copy link
Contributor

Separation of concerns. Gutenberg is a mon-repo with multiple packages.

This is the most compelling argument for me. Having tests for packages is good but I don't think we should be mocking things. The problem here is that we're trying to test edit-widgets, edit-post ... and these are applications and not really packages (in the sense of reusable npm packages) which is why I prefer e2e tests for these.

For me, at the moment we need mocking, the tests become less compelling.

@kevin940726 kevin940726 force-pushed the try/edit-widgets-integration-testing branch 2 times, most recently from d49b259 to 56db4eb Compare November 23, 2020 03:07
@kevin940726 kevin940726 force-pushed the try/edit-widgets-integration-testing branch 4 times, most recently from d831486 to 70a7ff6 Compare November 23, 2020 03:59
@kevin940726 kevin940726 force-pushed the try/edit-widgets-integration-testing branch from 70a7ff6 to f12afaa Compare November 23, 2020 05:39
@mcsf
Copy link
Contributor

mcsf commented Nov 23, 2020

👋 I don't have a whole lot to add to this conversation, but I want to highlight that, once we introduce systematic mocking, integration tests lose a lot of their essence as integration tests, and thus lose value.

We tend to think of testing methods based on a spectrum:

# more granular <----------------> more user-centered

unit tests <-> integration tests <-> end-to-end tests

The danger with this is that, when we seek something less granular and slightly more high-level than unit tests, we conflate that with integration tests. In reality, if we're talking about testing within a package, it's not really a question of integration. Rather, what we often want is a kind of unit testing operating on something more dev-facing, like package APIs. Perhaps we could think of these exported functions as second-order units — it's still unit testing, but in this context the "unit" is larger.

@gziolo
Copy link
Member

gziolo commented Nov 24, 2020

I just wanted to reshare some previous work in the same area to give you some more ideas.

There is also #26184 opened by @talldan where he proposes end to end tests for the standalone block editor. I suggested that it could use Storybook as a testing target. Have you considered a similar approach for the edit widgets package?

@ellatrix did some very promising explorations to write e2e like tests that use Storybook to run tests related to RichText functionality:

There were also explorations to use visual testing:

We have been playing with snapshot testing for components as well, You can check #18031 where Storyshots integration with Storybook was enabled. It was removed a few months later.

@kevin940726
Copy link
Member Author

Perhaps we could think of these exported functions as second-order units — it's still unit testing, but in this context the "unit" is larger.

I like to think of this as e2e tests, but the "end" here is the package users (developers or code that consumes this code) 😅.

I'm somehow convinced that maybe edit-widgets doesn't need integration testing or testing in isolation, all it needs is e2e testing with the whole app as its context.

I'm also convinced that maybe jsdom is not the optimal environment to run our tests. It's much faster in unit tests, but not by a large margin in e2e and integration tests. Though the argument still holds that jsdom is a lot easier to write and debug in my opinion.


For me, at the moment we need mocking, the tests become less compelling.

I agree. But I also feel like sometimes when mocking is still necessary, we should be using something more low-level like msw rather than mocking the fetch API or using page.setRequestInterception directly by ourselves. msw is a more universal solution and we can share the same mocks between tests/storybook/visual environments.

I want to propose a universal way of mocking things with msw, but maybe it's best to be in a separated and dedicated PR.

I also want to experiment a new way of querying and asserting things with RTL-like APIs in e2e tests. I can open a new PR for this too.


Thx for everyone's helps! I'm going to close this and open different PRs for different experiments mentioned above. As it's unlikely that doing integration testing in edit-widgets is going to be helpful in the current situation. I think it's best to avoid the potential confusion and bring some of the experiences here to different PRs.

@gziolo gziolo deleted the try/edit-widgets-integration-testing branch November 25, 2020 06:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Type] Automated Testing Testing infrastructure changes impacting the execution of end-to-end (E2E) and/or unit tests. [Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants