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

PoC for component testing #1137

Closed
wants to merge 3 commits into from

Conversation

giamir
Copy link
Contributor

@giamir giamir commented Oct 7, 2022

This PoC introduces a potential setup to enable component testing for Stacks.
Tests are executed via Web Test Runner in a browser context.
Web Test Runner (WTR) is configured to launch playwright and run tests in 3 different browser contexts: chromium, firefox and webkit. WTR is also using rollup to transform transitive dependencies which are not shipped as native ESM and to handle css imports. Furthermore WTR is using mocha as the default test runner.

The tests are written leveraging the open-wc testing package which ships with chai-dom as assertion library and provide some useful testing helpers to render html fixtures in the DOM.

Finally to query the DOM and simulate user interaction, tests are using respectively @testing-library/dom and @testing-library/user-event.

Testing library was built initially to run in a nodejs context and still uses some transitive dependencies which do not ship as ESM. We need rollup and its commonjs plugin in order to make the library work in a browser context (see the web-test-runner.config.mjs file for more details). See this open issue to get a better understanding of @testing-library/dom limitations in browser contexts.

As an alternative approach we could explore using playwright directly (no WTR) in combination with these Testing Library bindings.


The sample tests I have added in this PR (see the tooltip.test.ts file) are supposed to showcase the value of testing for catching unexpected regressions.
Not long ago we closed this bug without having to add any extra code because it was fixed as a side effects of our codebase evolving. In similar way the bug could represent itself with us being unaware: a test could help with that.
For convenience I have added to the codebase a version of the tooltip where the bug was still reproducible. You can switch to that version of the code in the ts/controllers/index.ts file and observe the tests failing catching the regression.


Please comment about this approach and mention any alternative you would be interested on exploring for making Stacks more robust and reliable as it evolves.

@giamir giamir added the do-not-merge Pull requests that are in progress and should not be merged yet label Oct 7, 2022
@netlify
Copy link

netlify bot commented Oct 7, 2022

Deploy Preview for stacks ready!

Name Link
🔨 Latest commit 414fb28
🔍 Latest deploy log https://app.netlify.com/sites/stacks/deploys/635a476bb372940008e753e2
😎 Deploy Preview https://deploy-preview-1137--stacks.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

Copy link
Contributor

@dancormier dancormier left a comment

Choose a reason for hiding this comment

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

This PoC introduces a potential setup to enable component testing for Stacks.

Woo! I'm excited. About time we got this rolling 🎉

Tests are executed via Web Test Runner in a browser context. Web Test Runner (WTR) is configured to launch playwright and run tests in 3 different browser contexts: chromium, firefox and webkit. WTR is also using rollup to transform transitive dependencies which are not shipped as native ESM and to handle css imports. Furthermore WTR is using mocha as the default test runner.

The tests are written leveraging the open-wc testing package which ships with chai-dom as assertion library and provide some useful testing helpers to render html fixtures in the DOM.

Finally to query the DOM and simulate user interaction, tests are using respectively @testing-library/dom and @testing-library/user-event.

Testing library was built initially to run in a nodejs context and still uses some transitive dependencies which do not ship as ESM. We need rollup and its commonjs plugin in order to make the library work in a browser context (see the web-test-runner.config.mjs file for more details). See this open issue

☝️That's not linking to where you intended

to get a better understanding of @testing-library/dom limitations in browser contexts.

I'll be honest: I was a little surprised to see so many new dependencies introduced to get testing rolling. I appreciate you outlining why they were added and I understand that a big part of it is the DOM creation/manipulation. I had assumed we could do something similar to what we do with @StackExchange/stacks-editor, but I realize now that the differing architectures necessitate different approaches to testing. Plus, they're all dev deps so whateverl!

As an alternative approach we could explore using playwright directly (no WTR) in combination with these Testing Library bindings.

FYI testing-library/playwright-testing-library#558

The sample tests I have added in this PR (see the tooltip.test.ts file) are supposed to showcase the value of testing for catching unexpected regressions. Not long ago we closed this bug without having to add any extra code because it was fixed as a side effects of our codebase evolving. In similar way the bug could represent itself with us being unaware: a test could help with that. For convenience I have added to the codebase a version of the tooltip where the bug was still reproducible. You can switch to that version of the code in the ts/controllers/index.ts file and observe the tests failing catching the regression.

Please comment about this approach and mention any alternative you would be interested on exploring for making Stacks more robust and reliable as it evolves.


I've updated the Codepen dependencies to use Stacks@1.3.0, which is before the tooltip flicker issue was resolved.

Comment on lines +22 to +29
expect(screen.queryByRole("tooltip")).to.be.null;

await user.hover(trigger);
const tooltip = await screen.findByRole("tooltip");
expect(tooltip).to.be.visible;

await user.unhover(trigger);
await waitForElementToBeRemoved(() => screen.queryByRole("tooltip"));
Copy link
Contributor

Choose a reason for hiding this comment

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

I tested out both the s-tooltip.ts and s-tooltip-1.3.0.ts being imported and looks like the tests pass/fail as we'd expect 🎉

With that said, I'm not sure why this one fails when using s-tooltip-1.3.0.ts

        await user.hover(trigger);
        const tooltip = await screen.findByRole("tooltip");
        expect(tooltip).to.be.visible;

I would expect tooltip to be visible under these circumstances. Can you help me understand why this fails? t's very likely that I'm missing something super obvious but I wanna make sure I understand this test and that it's behaving as we expect for the reasons we'd expect.

Copy link
Contributor Author

@giamir giamir Oct 27, 2022

Choose a reason for hiding this comment

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

If you look at the generated output you can spot that in this version we had another a11y bug where we were not updating correctly the aria-hidden attribute. When the user hovered on the tooltip aria-hidden remained set to true: you can check this also in the code pen you shared with me.
Perhaps this is a confirmation bias but this, in my opinion, highlights how powerful the testing library selectors are to spot a11y problems too.
This is the error output for me:

TestingLibraryElementError: Unable to find role="tooltip"

      Ignored nodes: comments, script, style
      <body>


        <div>


          <button
            aria-describedby="--stacks-s-tooltip-85tbv7uk"
            class="s-btn s-btn__filled"
            data-controller="s-tooltip"
            data-s-tooltip-placement="bottom-start"
            role="button"
          >

                      Hover tooltip popover

          </button>
          <div
            aria-hidden="true"
            class="s-popover s-popover__tooltip pe-none is-visible"
            data-popper-placement="bottom-start"
            id="--stacks-s-tooltip-85tbv7uk"
            role="tooltip"
            style="position: absolute; inset: 0px auto auto 0px; margin: 0px; transform: translate(0px, 48px);"
          >
            tooltip content
            <div
              class="s-popover--arrow"
              style="position: absolute; left: 0px; transform: translate(70px, 0px);"
            />
          </div>


        </div>
      </body>

You can see there that the aria-hidden is set to true and it should not. This is the same exact behavior we had for users until your refactor I think. 🙂

@giamir
Copy link
Contributor Author

giamir commented Oct 27, 2022

FYI testing-library/playwright-testing-library#558

@dancormier thanks for sharing this. It might be worth considering using exclusively playwright to cut down on dev dependencies.
I am personally leaning on "WTR+Testing-Library" simply because it creates a familiar dev experience for me (selfishly speaking - having used jest+JSDOM and Mocha+Sinon+Chai setups to write JS unit and component tests in the past). This motivation certainly doesn't justify the tool.

Playwright APIs remind me of classic E2E testing framework like Puppeteer, Selenium, etc.... although they are trying to up their game in the components space too: https://playwright.dev/docs/test-components
That stuff is still experimental but it goes into a similar direction to what WTR is doing - serving only the component(s) under test with on the fly source code transpilation and not the "whole page".
Some people use the playwright API for E2E tests and WTR for unit testing (in a real browser context -> more confidence than JSDOM or similar). https://twitter.com/auniverseaway/status/1581320051684044800

By the way since we have been complaining a bit about Backstop as well in the past few days I thought I would add an extra commit showing how we could leverage the same exact WTR setup to run visual regression too. It's an option we could certainly evaluate:
414fb28
Here is some extra info: modernweb-dev/web#427

@dancormier
Copy link
Contributor

dancormier commented Nov 1, 2022

FYI testing-library/playwright-testing-library#558

@dancormier thanks for sharing this. It might be worth considering using exclusively playwright to cut down on dev dependencies. I am personally leaning on "WTR+Testing-Library" simply because it creates a familiar dev experience for me (selfishly speaking - having used jest+JSDOM and Mocha+Sinon+Chai setups to write JS unit and component tests in the past). This motivation certainly doesn't justify the tool.

I'm into the "WTR+Testing-Library" approach for now. I think trying to squeeze our testing exclusively into playwright would be opting for simplicity at the cost of utility.

Playwright APIs remind me of classic E2E testing framework like Puppeteer, Selenium, etc.... although they are trying to up their game in the components space too: https://playwright.dev/docs/test-components That stuff is still experimental but it goes into a similar direction to what WTR is doing - serving only the component(s) under test with on the fly source code transpilation and not the "whole page". Some people use the playwright API for E2E tests and WTR for unit testing (in a real browser context -> more confidence than JSDOM or similar). https://twitter.com/auniverseaway/status/1581320051684044800

By the way since we have been complaining a bit about Backstop as well in the past few days I thought I would add an extra commit showing how we could leverage the same exact WTR setup to run visual regression too. It's an option we could certainly evaluate: 414fb28 Here is some extra info: modernweb-dev/web#427

Oh, I'm excited to check this out! Thanks for adding it.

Copy link
Contributor

@dancormier dancormier left a comment

Choose a reason for hiding this comment

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

Should we be committing visual regression testing screenshots to the repo? I'm open to considering it since the images are per-test and isolated to a given example component as opposed the entirety of each docs page.

With that said, we've regretted including images from Backstopjs and I worry that including the images from WTR will bloat our git history as we add more tests.

@giamir
Copy link
Contributor Author

giamir commented Nov 2, 2022

Should we be committing visual regression testing screenshots to the repo?

Storing images in the repo is probably the most pragmatic and simpler approach.
That said if you have already had bad experiences with that approach in the past we could look into the git-lfs addon which is supported by Github. It should be pretty straightforward to setup.
https://docs.github.com/en/repositories/working-with-files/managing-large-files/configuring-git-large-file-storage

The goal should be to be able to run tests locally and in CI against the most updated baseline images. How we achieve that is secondary (there are also 3rd party commercial services which store the baseline images for you in their cloud).

The setup we currently have with backstop (not storing baseline images) is suboptimal and time expensive: contributors are not incentivised enough to run the test suite before they push their changes (which in turn diminish the value-cost ratio of our tests).

@b-kelly when you have some time (there is no rush) I'd like to hear also your opinion on the matter. 🙂

@giamir
Copy link
Contributor Author

giamir commented Nov 28, 2022

Archiving this PoC Testing PR in favor of this one: #1194

@giamir giamir closed this Nov 28, 2022
@b-kelly b-kelly deleted the gbuoncristiani/introduce-testing-setup branch January 18, 2023 19:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
do-not-merge Pull requests that are in progress and should not be merged yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Tooltip flickers when hovering SVG
2 participants