Feat: prototype unit testing #712

mmiask · 2020-11-09T17:46:31Z

Closes: #660

Setup of react-testing-library and its utilities
Configuration of jest config file to handle node modules mappings
Added data-testid attributes
Wrote unit tests for Try It Out header component
Configured CircleCI job to run tests and clear jest cache beforehand (due to recurring errors in loading test files; solution: ts-jest 26.1.2 - "cannot find source file" kulshekhar/ts-jest#1506)

bumped @testing-library/react

add script to package.json

netlify · 2020-11-09T17:50:30Z

Deploy preview for stoplight-elements ready!

Built with commit 0f43b92

https://deploy-preview-712--stoplight-elements.netlify.app

added @testing-library/dom added @testing-library/user-event added a test case

marcelltoth

The setup is nice, it works great!

I'm not so much a fan of the tests however. I know they are just some examples, but they are not really good examples right now IMO. Let's tidy them up together!

As they stand now, both the selectors and the assertions use a lot of artificial artifacts such as the copy, data-* attributes or test ids. These should be used as the last resort and when you use them you should question whether the thing is worth testing in the first place. We should always keep testing goals & pain/reward ratios in mind.

I offered you some tips on how I would make that one test nicer, but you can apply the same principles to most of the other selectors and assertions, too.

But it really is very difficult to come up with a good testing approach in regards to this component. And as I said, when encountering this, one should stop and rethink. So digging further down I think I found what the real problem is here: The component you picked doesn't have much behavior to test. If you open its source code you can see that's its completely static, does nothing - except for that popup that may very well be the only valid thing to test here. It's almost static HTML.

I don't think these static components are worth testing at all, because what can you assert? That it renders what's written into the source code? That doesn't offer any added value, does it? I think in general we are looking to test behavior and very little presentation, so maybe it all only has to do with the component choice. That's one of the reasons I suggested to do an overall test of API.

Having said that, if you can clean this one up - even if you only keep a single test - that's perfectly good enough as well to complete the connected issue. The issue says you can test anything, what I would not like however, is to merge an example of doing the wrong thing. Having a single very simple but nicely written example is much much better.

marcelltoth · 2020-11-13T09:26:06Z

.circleci/config.yml

@@ -17,6 +17,7 @@ jobs:
      - run: yarn --frozen-lockfile
      - run: yarn type-check
      - run: yarn lint
+      - run: yarn test.clear


Why do we need this?

This is related to kulshekhar/ts-jest#1506 - I encountered an error when trying to run tests with jest a couple of times and clearing cache helped. We could get rid of for now and see if it breaks in CircleCI frequently, but it only adds about 2 seconds to a job.

It cannot affect CircleCI because it's a new container each time, with an empty cache - that's also why it takes 2s.
This justifies having the yarn command however, if it is useful for you locally.

The issue you linked to is closed however ("v26.1.4 is out, I will close this issue"), so you could try upgrading ts-jest (by upgrading jest itself) and that should fix your local problems as well.

marcelltoth · 2020-11-13T10:14:48Z

packages/elements/src/components/TryIt/__tests__/header.test.tsx

+  expect(elem.innerHTML).toContain('data-icon="magic"');
+  expect(elem.innerHTML).toContain('data-icon="question-circle"');


This is implementation-dependent and unimportant

marcelltoth · 2020-11-13T10:15:46Z

packages/elements/src/components/TryIt/__tests__/header.test.tsx

+    await screen.findByText(
+      "Send HTTP requests to your API, or one of our mock servers, to see how it's going to respond. View the docs",
+    ),


Testing the copy is rarely a good idea either. The only thing it does is makes it more annoying to change it down the road, without providing any real safety.

But testing whether the popover works is a good one!

Now on how to select it? One really underrated and underutilized way of selecting things is by accessibility attributes (ARIA).

You may be familiar already that HTML5 includes many new "semantic" tags that behave the same as a regular div, but mean a slightly different thing. section, main, article, etc. Since HTML5, the official goal of HTML is to describe semantics, not to prescribe appearance or behavior. (We have CSS and JS for those, respectively.) We should utilize this semantic part of HTML more than we do right now, but that's another matter.

In addition to these, a more in-depth way to describe a web-page is using the HTML accessibility attributes: https://www.w3.org/TR/wai-aria-1.1/ & https://developer.mozilla.org/en-US/docs/Learn/Accessibility/WAI-ARIA_basics

While these are mainly designed to provide accessibility to websites - help screen readers and such - they implicitly let you describe the semantics of your site really well, the very semantics that you want to test. I tend to think of these as tools aiding testability and comprehension, and think of the accessibility improvement as a nice side-effect.

In this case using the tooltip role would be a perfect fit, then you could use the screen.findByRole selector to find the element and instead of asserting on the contents you could assert e.g. toBeVisible

Testing the copy is rarely a good idea either. The only thing it does is makes it more annoying to change it down the road, without providing any real safety.

I agree and will get rid of it

mmiask · 2020-11-13T15:03:52Z

@marcelltoth

I don't think these static components are worth testing at all, because what can you assert?

I agree, let's drop the checking of displayed text

That's one of the reasons I suggested to do an overall test of API.

I wanted to test that as well, but ran into a situation when react-testing-library was asserting elements in empty skeleton of API component, before the data was loaded. It seemed too much of a hustle ATM comparing to just testing a simple component like Try It header, and that's why I decided to switch.

marcelltoth · 2020-11-13T15:27:50Z

I wanted to test that as well, but ran into a situation when react-testing-library was asserting elements in empty skeleton of API component, before the data was loaded. It seemed too much of a hustle ATM comparing to just testing a simple component like Try It header, and that's why I decided to switch.

Understood. 👍 Then let's just narrow this down to as much as we can, I'm fine with only having that single test that asserts that the tooltip is visible on hover. I don't think there's any more behavior in this component at all, but if you think so, sure, let's add that, too.

mmiask · 2020-11-13T15:33:40Z

@marcelltoth bear in mind that there's still a need to add a data-testid attribute to question-icon since its role is not unique in the component

marcelltoth · 2020-11-13T16:10:23Z

@marcelltoth bear in mind that there's still a need to add a data-testid attribute to question-icon since its role is not unique in the component

I guess it's fine for now. It's an example afterall, we won't be testing Popups like this much anymore

…otype-unit-testing

removed unused data-testid attribute

marcelltoth

The test looks quite pretty and concise now, I love it! 🎉

You could remove all the --clearCache-related lines, they shouldn't be needed anymore. If someone really wants to run it they can always run yarn test --clearCache or yarn jest --clearCache or yarn workspace run jest --clearCache if you want to run it everywhere. We shouldn't create yarn commands for everything we might need to do, only the most frequent.

Other than that, good to go! 🚀

mmiask added 4 commits November 9, 2020 11:27

feat: init commit

2ac25b5

fix: added moduleNameMapper in jest config

c577d31

feat: simple unit test

72625d8

bumped @testing-library/react

feat: configure CircleCI job

fe41b14

add script to package.json

mmiask added 4 commits November 10, 2020 11:33

fix: remove failing test

af91306

feat: add script to element-utils package.json

30cdb3f

feat: changing steps order in 'lint-and-check'

375a1b1

feat: finished unit tests for Try It Out header

b2dc2a6

added @testing-library/dom added @testing-library/user-event added a test case

mmiask self-assigned this Nov 10, 2020

mmiask added the team/undefined label Nov 10, 2020

mmiask requested a review from a team November 10, 2020 14:35

mmiask marked this pull request as ready for review November 10, 2020 14:35

mmiask requested a review from philsturgeon as a code owner November 10, 2020 14:35

marcelltoth suggested changes Nov 13, 2020

View reviewed changes

marcelltoth mentioned this pull request Nov 13, 2020

test: upgrade jest to 26 #721

Merged

mmiask added 3 commits November 13, 2020 17:36

feat: addressed review comments

71c3697

Merge branch 'beta' of github.com:stoplightio/elements into feat/prot…

78419c1

…otype-unit-testing

fix: yarn.lock

5959380

mmiask requested a review from marcelltoth November 13, 2020 16:45

fix: removed yarn test.clear job from CircleCI

0701c09

removed unused data-testid attribute

marcelltoth approved these changes Nov 13, 2020

View reviewed changes

fix: removed jest --clearCache completely

0f43b92

mmiask merged commit 26440d3 into beta Nov 14, 2020

mmiask deleted the feat/prototype-unit-testing branch November 14, 2020 20:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feat: prototype unit testing #712

Feat: prototype unit testing #712

mmiask commented Nov 9, 2020 •

edited

Loading

netlify bot commented Nov 9, 2020 •

edited

Loading

marcelltoth left a comment

marcelltoth Nov 13, 2020

mmiask Nov 13, 2020

marcelltoth Nov 13, 2020

marcelltoth Nov 13, 2020

marcelltoth Nov 13, 2020

marcelltoth Nov 13, 2020

mmiask Nov 13, 2020

mmiask commented Nov 13, 2020

marcelltoth commented Nov 13, 2020

mmiask commented Nov 13, 2020

marcelltoth commented Nov 13, 2020

marcelltoth left a comment

		expect(elem.innerHTML).toContain('data-icon="magic"');
		expect(elem.innerHTML).toContain('data-icon="question-circle"');

Feat: prototype unit testing #712

Feat: prototype unit testing #712

Conversation

mmiask commented Nov 9, 2020 • edited Loading

netlify bot commented Nov 9, 2020 • edited Loading

marcelltoth left a comment

Choose a reason for hiding this comment

marcelltoth Nov 13, 2020

Choose a reason for hiding this comment

mmiask Nov 13, 2020

Choose a reason for hiding this comment

marcelltoth Nov 13, 2020

Choose a reason for hiding this comment

marcelltoth Nov 13, 2020

Choose a reason for hiding this comment

marcelltoth Nov 13, 2020

Choose a reason for hiding this comment

marcelltoth Nov 13, 2020

Choose a reason for hiding this comment

mmiask Nov 13, 2020

Choose a reason for hiding this comment

mmiask commented Nov 13, 2020

marcelltoth commented Nov 13, 2020

mmiask commented Nov 13, 2020

marcelltoth commented Nov 13, 2020

marcelltoth left a comment

Choose a reason for hiding this comment

mmiask commented Nov 9, 2020 •

edited

Loading

netlify bot commented Nov 9, 2020 •

edited

Loading