feat(configureTestIdAttribute): add ability to override data-testid [generic config] #164

RoystonS · 2018-11-28T15:14:35Z

N.B. This is one of two alternative PRs with slightly differing configuration APIs. This one contains a generic configuration API exposing one configuration function taking an object which could be extended for future use. That may desirable or not, hence the two PRs.

What:

Introduce an API to allow data-testid to be overridden without consumers needing
to construct and maintain a set of higher level decorator functions to override the various 'xxxByTestId' functions as exposed in various places.

Why:

The existing API, where data-testid cannot be changed at the low level, requires a significant amount of effort to customise the higher-level APIs. See issue #162 for more discussion.

How:

Ultimately we needed to parameterise what was previously some hardcoded strings in queries.js. I would have preferred to keep the changes local to queries.js, but we assume elsewhere that all exports from queries.js are queries, and I didn't want to alter that. So I added a simple config.js module to store the actual configuration, and queries.js references that and index.js exposes the tiny configuration API.

Checklist:

Documentation
Tests
Ready to be merged
Added myself to contributors table

Adds the configureTestIdAttribute function that enables the overriding of data-testid without having to write and maintain higher-level decoration to replace all the ByTestId queries in various places.

This was a typo whilst producing the two PRs.

kentcdodds

This is actually really good. I'm in favor but would love to hear what other people have to say.

alexkrolick · 2018-11-29T01:34:12Z

src/config.js

+  }
+}
+export function getTestIdAttribute() {
+  return config.testIdAttribute


This looks good to me. My only nit is that I don't think there needs to be a special method to grab just this attribute; export the whole config object or a getConfig function instead.

Configure could also take an update function:

configure(currentConfig => { return {...newConfig} })

This would be nice if you want to read and extend subobjects or arrays (like the current set of queries) without importing the default value into the host file.

This looks good to me. My only nit is that I don't think there needs to be a special method to grab just this attribute; export the whole config object or a getConfig function instead.

This comes from of some of my type-safety background. It's easy to get a silent failure from getConfig().mistypedConfigKeyName but getMistypedConfigKeyFunction() will always blow. But in this case the code will be protected by a unit test so that shouldn't be an issue. I'll tweak it to a getConfig function as that's more flexible than merely exposing the whole config.

davidgilbertson · 2018-11-29T08:43:33Z

This is great, nice one @RoystonS .

Just a note (to self, you or @kentcdodds), the "What if my project already uses data-test-id?" section in react-testing-library can be replaced with a link to this, once merged.

RoystonS · 2018-11-29T08:44:38Z

This is great, nice one @RoystonS .

Just a note (to self, you or @kentcdodds), the "What if my project already uses data-test-id?" section in react-testing-library can be replaced with a link to this, once merged.

Absolutely. Very much looking forward to being able to do that!

This commit prepares the way for configure() to be used other than just for `testIdAttribute`: it exposes the whole config internally and upgrades the configure() function so that it can take a function that. I've introduced explicit tests for this more complex configuration API and a new documentation section for it. The documentation for the function-based configure() API could be improved with a real example once there is a real key for which it would be useful.

RoystonS · 2018-11-29T09:19:13Z

I've added another commit for @alexkrolick's extended configuration use case. That required a new set of tests and a new documentation section.

RoystonS · 2018-11-29T16:09:44Z

I've made the usual sacrifices to eslint to make it shut up; this is now ready for review.

kentcdodds

Looks super! Just a few things :)

kentcdodds · 2018-11-29T16:40:48Z

README.md

+Configuration options:
+
+`testIdAttribute`
+: The attribute used by `getByTestId` and related queries. Defaults to `data-testid`. See [`getByTestId`](#getbytestid).


Can this be placed on the same line so there's no space in the rendered output?

Hmm. Might be another prettier funsie. I'm sure I put them on the same line originally. I'll have a go...

kentcdodds · 2018-11-29T16:44:57Z

src/__tests__/config.js

+  })
+
+  describe('configure', () => {
+    test('merges a delta rather than replacing the whole config', () => {


Hmmm... I think the fact that you can add random properties to the configuration is an implementation detail and not necessarily a feature we care to support. By that I mean, if we change the implementation in the future to only allow you to configure keys of things we support then I don't want to have to bother with fixing these tests.

So if we could make only assertions for the features we care to support that would be great.

More specifically, this test is fine, so long as we change the assertion to:

expect(conf).toMatchObject({testIdAttribute: 'data-testid'})

The fact that newKey exists as well is a coincidence and not necessarily something we need to assert in our tests.

kentcdodds · 2018-11-29T16:45:41Z

src/__tests__/config.js

+    test('passes existing config out to config function', () => {
+      // Create a new config key based on the value of an existing one
+      configure(existingConfig => ({
+        newKey: `${existingConfig.testIdAttribute}-derived`,


Rather than newKey in this test, we could set this to the testIdAttribute and get the same value out of the test without asserting on a non-feature.

Yup, will update tests so they don't create random properties. Would have been easier to test had there been two supported properties from the start.

kentcdodds

Perfect 💯

kentcdodds · 2018-11-29T17:13:18Z

🎉 This PR is included in version 3.13.0 🎉

The release is available on:

npm package (@latest dist-tag)
GitHub release

Your semantic-release bot 📦🚀

RoystonS · 2018-11-29T17:14:39Z

Very much appreciated!

feat(configureTestIdAttribute): add ability to override data-testid

b8c0217

Adds the configureTestIdAttribute function that enables the overriding of data-testid without having to write and maintain higher-level decoration to replace all the ByTestId queries in various places.

RoystonS mentioned this pull request Nov 28, 2018

Overriding data-testid requires a lot of work, and can't be done completely #162

Closed

Fix broken export

a9c61a1

This was a typo whilst producing the two PRs.

kentcdodds previously approved these changes Nov 28, 2018

View reviewed changes

alexkrolick reviewed Nov 29, 2018

View reviewed changes

RoystonS dismissed kentcdodds’s stale review via eaedbde November 29, 2018 09:17

Fix eslint complaint

26cd837

kentcdodds previously approved these changes Nov 29, 2018

View reviewed changes

Tidy README & don't rely on unsupported API in tests

38b607e

RoystonS dismissed kentcdodds’s stale review via 38b607e November 29, 2018 17:03

kentcdodds approved these changes Nov 29, 2018

View reviewed changes

kentcdodds merged commit 879bf94 into testing-library:master Nov 29, 2018

RoystonS deleted the pr/configure-testid-generic-config branch November 29, 2018 17:08

kentcdodds added the released label Nov 29, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(configureTestIdAttribute): add ability to override data-testid [generic config] #164

feat(configureTestIdAttribute): add ability to override data-testid [generic config] #164

RoystonS commented Nov 28, 2018

kentcdodds left a comment

alexkrolick Nov 29, 2018

alexkrolick Nov 29, 2018

RoystonS Nov 29, 2018 •

edited

Loading

davidgilbertson commented Nov 29, 2018

RoystonS commented Nov 29, 2018

RoystonS commented Nov 29, 2018

RoystonS commented Nov 29, 2018

kentcdodds left a comment

kentcdodds Nov 29, 2018

RoystonS Nov 29, 2018

kentcdodds Nov 29, 2018

kentcdodds Nov 29, 2018

RoystonS Nov 29, 2018

kentcdodds left a comment

kentcdodds commented Nov 29, 2018

RoystonS commented Nov 29, 2018

feat(configureTestIdAttribute): add ability to override data-testid [generic config] #164

feat(configureTestIdAttribute): add ability to override data-testid [generic config] #164

Conversation

RoystonS commented Nov 28, 2018

kentcdodds left a comment

Choose a reason for hiding this comment

alexkrolick Nov 29, 2018

Choose a reason for hiding this comment

alexkrolick Nov 29, 2018

Choose a reason for hiding this comment

RoystonS Nov 29, 2018 • edited Loading

Choose a reason for hiding this comment

davidgilbertson commented Nov 29, 2018

RoystonS commented Nov 29, 2018

RoystonS commented Nov 29, 2018

RoystonS commented Nov 29, 2018

kentcdodds left a comment

Choose a reason for hiding this comment

kentcdodds Nov 29, 2018

Choose a reason for hiding this comment

RoystonS Nov 29, 2018

Choose a reason for hiding this comment

kentcdodds Nov 29, 2018

Choose a reason for hiding this comment

kentcdodds Nov 29, 2018

Choose a reason for hiding this comment

RoystonS Nov 29, 2018

Choose a reason for hiding this comment

kentcdodds left a comment

Choose a reason for hiding this comment

kentcdodds commented Nov 29, 2018

RoystonS commented Nov 29, 2018

RoystonS Nov 29, 2018 •

edited

Loading