Add docs about decision around, and alternatives to, testID #35

lemonmade · 2018-07-10T18:35:33Z

Does what it says on the tin. I wanted to get this down into words so people understand why we are mostly opposed to the use of this attribute, provide some historical context for why we have so many in the existing Web app, and give people alternatives for the common patterns I have seen.

cc/ @Shopify/web-foundation

TzviPM

looks great. Should we also mention the issue of tracking (trekkie, etc.) that seemingly prompted us to move away from testIDs?

TzviPM · 2018-07-10T18:39:19Z

Best practices/Testing.md

+  it('includes the children in the markup', () => {
+    const children = <div>Contents</div>;
+    const card = mount(<Card>{children}</Card>);
+    expect(card).toContainReact(children);


TzviPM · 2018-07-10T18:41:44Z

Best practices/Testing.md

+
+    To make this approach even easier, our [`enzyme-utilities` package](https://github.com/Shopify/quilt/tree/master/packages/enzyme-utilities) provides a `findById` utility function.
+
+  * If neither of the above apply, filter the set of matched components based on the properties that are unique to them. If no properties are unique, fall back to the index of the item in the matched set.


this wording is a bit confusing to me. To confirm, if you can't make it into a separate component, and a unique ID doesn't make sense, then simply find all instances of the component and filter them by some other unique thing like text or a prop. If there are no unique attributes that make sense, then locate the item by its index (ex. the second modal)?

Yes, that is exactly correct (text is always a prop of the component, but I wanted to try to not tie this entire section just to React)

TzviPM · 2018-07-10T18:42:00Z

Decision records/04 - We do not use test IDs in tests.md

@@ -0,0 +1,49 @@
+# We do not use test IDs in tests


maybe just We do not use test IDs ?

loic-d · 2018-07-10T18:54:01Z

We ran into this problem with our DiscountSummary component that renders different summary lines to describe discount characteristics to merchants. However, based on the type of discount, the outputted markup for a given line will be different. We used testID to clearly identify which part of the JSX was rendered.

I think that removing testID would have forced us to maybe split our component in sub-components per summary lines, or just assert that the text at a given line index was correct. At the end of the day, we just want to know if the UI matches the UX spec, but don't really care how we got there.

The proposed changes make sense 👍 but we will definitely have to do some refactoring in multiple places to respect this new best practice.

loic-d · 2018-07-10T18:54:28Z

Best practices/Testing.md

+  it('includes the children in the markup', () => {
+    const children = <div>Contents</div>;
+    const card = mount(<Card>{children}</Card>);
+    expect(card).toContainReact(children);


Really nice, didn't know you could do that.

rox163 · 2018-07-10T20:18:58Z

Love it! Simple yet in-depth explanation of a really important practice.

cartogram

👏

cartogram · 2018-07-11T03:25:29Z

Best practices/Testing.md

+* Markup that has an important effect on the visual output of a component, but which have no other defining characteristics (for example, an element on which we apply a particular class name or inline styles).
+* A child component that is rendered multiple times, where the developer wishes to easily disambiguate between the instances (for example, a component that renders multiple `Modal`s from Polaris React).
+
+We would like to avoid test IDs for handling the cases above, as make it easy to reach into your component in ways that unnecessarily lock down the implementation. You can typically avoid test IDs by using one of the following strategies:


as make it easy => as they make it easy

cartogram · 2018-07-11T03:34:55Z

Decision records/04 - We do not use test IDs in tests.md

+
+## Description
+
+Developers sometimes feel the need to add "hooks" that allow their tests to easily find parts of the component subtree and assert details about those subtrees in tests. In the React ecosystem, this is a key recommendation of an alternative to Enzyme (our preferred React testing library), [`react-testing-library`/ `dom-testing-library`](https://github.com/kentcdodds/dom-testing-library/blob/master/README.md#faq).


That second sentence is distracting and does not really add anything.

cartogram · 2018-07-11T03:40:38Z

Decision records/04 - We do not use test IDs in tests.md

+In building the initial version of Shopify Web, we took several steps to encourage and facilitate the use of test IDs:
+
+* A [babel plugin](https://github.com/lemonmade/babel-plugin-react-test-id) to remove the unnecessary attribute in production
+* Allowing the `testID` prop implicitly with TypeScript and adding a `findByTestId` utility to Shopify Web


This is 2 separate bullets, but its fine.

cartogram · 2018-07-11T03:43:37Z

Decision records/04 - We do not use test IDs in tests.md

+* A [babel plugin](https://github.com/lemonmade/babel-plugin-react-test-id) to remove the unnecessary attribute in production
+* Allowing the `testID` prop implicitly with TypeScript and adding a `findByTestId` utility to Shopify Web
+
+These were originally introduced to facilitate differentiation between the same React component rendered multiple times within a tree (specifically, to target multiple `TextField` components rendered by a card in the product section of the admin). No analysis of alternatives was performed at the time. The cases in which this attribute was used expanded and, as of July 10, 2018, it is used 280 times in the Web codebase.


I don't think you need the part in parens. And maybe Web should be Shopify Web to match when it is mentioned above.

cartogram · 2018-07-11T03:46:07Z

Decision records/04 - We do not use test IDs in tests.md

+
+## Decision
+
+`testID` provides some benefits that have made it attractive to developers working in the Web stack:


Web => Shopify Web

cartogram · 2018-07-11T03:49:49Z

Decision records/04 - We do not use test IDs in tests.md

+* You must prevent console warnings that React prints by default complaining about an unexpected `testID` prop on DOM elements
+* You must be sure to include our custom Babel plugin to strip the attribute in production builds
+
+Additionally, in reviewing the use of `testID` in Web, we found that the ways it were used were better addressed using other strategies. We have detailed common patterns we found (along with alternative strategies to `testID`) in our [testing best practices](../Best%20practices/Testing.md#test-ids), but in summary, we found that `testID` was used to:


Web => Shopify Web (or change them all to just Web). I don't have a preference other than that it is consistent.

Update changelog for release 5.1.0

Use full module name for presets

Add docs about test ID

19555de

lemonmade requested review from cartogram, GoodForOneFare, TzviPM and marutypes July 10, 2018 18:35

TzviPM approved these changes Jul 10, 2018

View reviewed changes

loic-d reviewed Jul 10, 2018

View reviewed changes

cartogram approved these changes Jul 11, 2018

View reviewed changes

PR feedback

798a044

lemonmade merged commit 9885149 into master Jul 11, 2018

lemonmade deleted the test-id branch July 11, 2018 13:09

BPScott pushed a commit that referenced this pull request Mar 25, 2020

Tests for jquery-dollar-sign-reference + computed properties (#35)

75fefe2

BPScott pushed a commit that referenced this pull request Mar 25, 2020

Merge pull request #35 from Shopify/changelog-5.1.0

f54e89b

Update changelog for release 5.1.0

BPScott added a commit that referenced this pull request Mar 25, 2020

Merge pull request #35 from Shopify/readme-update

1798ed9

Use full module name for presets

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add docs about decision around, and alternatives to, testID #35

Add docs about decision around, and alternatives to, testID #35

lemonmade commented Jul 10, 2018

TzviPM left a comment

TzviPM Jul 10, 2018

TzviPM Jul 10, 2018

lemonmade Jul 11, 2018

TzviPM Jul 10, 2018

loic-d commented Jul 10, 2018

loic-d Jul 10, 2018

rox163 commented Jul 10, 2018

cartogram left a comment

cartogram Jul 11, 2018

cartogram Jul 11, 2018

cartogram Jul 11, 2018

cartogram Jul 11, 2018

cartogram Jul 11, 2018

cartogram Jul 11, 2018


		To make this approach even easier, our [`enzyme-utilities` package](https://github.com/Shopify/quilt/tree/master/packages/enzyme-utilities) provides a `findById` utility function.

		* If neither of the above apply, filter the set of matched components based on the properties that are unique to them. If no properties are unique, fall back to the index of the item in the matched set.


		## Description

		Developers sometimes feel the need to add "hooks" that allow their tests to easily find parts of the component subtree and assert details about those subtrees in tests. In the React ecosystem, this is a key recommendation of an alternative to Enzyme (our preferred React testing library), [`react-testing-library`/ `dom-testing-library`](https://github.com/kentcdodds/dom-testing-library/blob/master/README.md#faq).


		## Decision

		`testID` provides some benefits that have made it attractive to developers working in the Web stack:

Add docs about decision around, and alternatives to, testID #35

Add docs about decision around, and alternatives to, testID #35

Conversation

lemonmade commented Jul 10, 2018

TzviPM left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

loic-d commented Jul 10, 2018

Choose a reason for hiding this comment

rox163 commented Jul 10, 2018

cartogram left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment