feat: Depreciate toBeInTheDOM with replacement #40

smacpherson64 · 2018-07-14T22:07:55Z

What:
Deprecating toBeInTheDOM
Adding toBeInTheDocument

Why:
Aiming to implement desires from conversation in issue #34

How:
Added a deprecate function to warn of deprecation
Added jsdoc to types to label as deprecated
Added toBeInTheDocument function
Updated documentation to have deprecated notices
Added toBeInTheDocument to documentation

Checklist:

@gnapse, @jgoz, and @kentcdodds Here is the first attempt to do the changes we discussed in #34, when you have time please review. :-) !

jgoz

Implementation of toBeInTheDocument looks good, just a couple minor suggestions from me.

jgoz · 2018-07-14T23:25:54Z

src/utils.js

+  console.warn(message)
+}
+
+function getDepreciationMessage(name, replacementText) {


Does this need to be exposed as a separate function at all? I know it's used in the tests, but I would rather see the full message typed out in the tests than use this function in both spots. Otherwise we never really test the output of this function.

My preference would be to remove this function and inline the message creation in deprecate:

function deprecate(name, replacementText) { // Notify user that they are using deprecated functionality. // eslint-disable-next-line no-console console.warn( `Warning: ${name} has been deprecated and will be removed in future updates.`, replacementText ) }

Sounds good, I will aim to remove this and update the check to use the text.

jgoz · 2018-07-14T23:26:44Z

src/__tests__/utils.js

+test('deprecate', () => {
+  const spy = jest.spyOn(console, 'warn').mockImplementation(() => {})
+
+  const messageWithReplacement = getDepreciationMessage('test', 'test')


As I commented below, I think we should type out the full expected message here, rather than rebuilding it again with the function.

jgoz · 2018-07-14T23:30:17Z

src/to-be-in-the-document.js

+  checkHtmlElement(element, toBeInTheDocument, this)
+
+  return {
+    pass: document.documentElement.contains(element),


Should we check whether document and document.documentElement exist? I know jsdom will have them, but if I accidentally used this matcher in the node environment, it would be nice to get a descriptive error message rather than "cannot read property 'documentElement' of undefined".

Hrm, sounds good, agreed on developer experience. It would be nice to get a clear error. Does the following sound good @jgoz?

document is undefined on global but is required for the toBeInTheDocument matcher.

documentElement is undefined on document but is required for the toBeInTheDocument matcher.

👍 the first message should be good enough for both cases.

smacpherson64 · 2018-07-14T23:37:21Z

@jgoz Thanks for the great feedback!

jgoz

Changes & proposed changes look good to me! Just one remaining nitpick in the readme.

(Btw, I'm heading on vacation for a few days and won't have internet connectivity during that time.)

jgoz · 2018-07-15T06:04:38Z

README.md

 ```

-This allows you to assert whether an element has content or not.
+This allows you to assert whether an element present in the document or not.


Missing is before present

Nice find! Kk, fixed. (I hope you have an awesome vacation!)

smacpherson64 · 2018-07-15T17:14:28Z

@jgoz: here is an update on better error message on document for developer experience:

TDLR

I am not sure that jest-dom can be used without the jsdom environment. I get an error when trying to test without document:

... jest-dom\node_modules\jsdom\lib\jsdom\browser\Window.js:598
      window.document.addEventListener("load", () => {

I believe it is pulled in here:
https://github.com/gnapse/jest-dom/blob/master/jest.config.js#L4

Details

To get a better error message when the document is missing, I created a checkDocumentKey utility similar to checkHtmlElement:

function checkDocumentKey(key, matcherFn) {
  if (typeof document === 'undefined') {
    throw new InvalidDocumentError(
      `document is undefined on global but is required to use ${
        matcherFn.name
      }.`,
      matcherFn,
    )
  }

  if (typeof document[key] === 'undefined') {
    throw new InvalidDocumentError(
      `${key} is undefined on document but is required to use ${
        matcherFn.name
      }.`,
      matcherFn,
    )
  }
}

I can successfully get all the tests to pass however, getting coverage up to 100% requires testing without the document. When I remove the document from global, the util test throws an error.

Test that Caused Error

describe('checkDocumentKey without document', () => {
  beforeEach(() => {
    global.previousDocument = {...document}
    delete global.document
  })

  test('checkDocumentkey', () => {
    expect(() =>
      checkDocumentKey('documentElement', matcherMock),
    ).toThrowError()
  })

  afterEach(() => {
    global.document = global.previousDocument
    delete global.previousDocument
  })
})

Error

... jest-dom\node_modules\jsdom\lib\jsdom\browser\Window.js:598
      window.document.addEventListener("load", () => {

smacpherson64 · 2018-07-15T17:39:33Z

Well, I was wrong about getting coverage to 100%, haha there is always another way. @jgoz I have updated the concept and it should now check for undefined document or a specific key on document.

I am still not sure if jest-dom can run without jsdom.

gnapse · 2018-07-16T20:20:17Z

README.md

-expect(queryByTestId(container, 'not-empty')).not.toBeEmpty()
+// <span data-testid="count-value">2</span>
+expect(queryByTestId(container, 'count-value').toBeInTheDocument()
+expect(queryByTestId(container, 'count-value1')).not.toBeInTheDocument()


I think that expect(nullOrUndefined).not.toBeInTheDocument() would fail before even having the chance to actually check for presence in the document. So this second example is not accurate because queryByTestId(container, 'count-value1') will return null or undefined, and neither of those is a html or svg element.

So in this sense, it's not a direct replacement for toBeInTheDOM, and I think that's a good thing. The migration path is not simply "replace all uses of toBeInTheDOM with toBeInTheDocument.

Not sure though on how to approach this in the README to put examples of both a positive and a negative test case passing. And also I think we need a test that asserts that toBeInTheDocument indeed fails when given null or undefined.

Sounds Good! I will work to take care of these!

Thank you @gnapse!

smacpherson64 · 2018-07-17T20:40:21Z

Hey @gnapse!

I believe the following should be complete:

Updated the README to have better examples for toBeInTheDocument (using examples from the tests).
Added a disclaimer aimed to emphasize that toBeInTheDocument is not a direct replacement of toBeInTheDOM.
Added null and undefined tests

Please let me know of any other change you desire!

gnapse · 2018-07-18T13:22:58Z

README.md

+// const htmlElement = document.querySelector('[data-testid="html-element"]')
+// const svgElement = document.querySelector('[data-testid="html-element"]')
+// const detachedElement = document.createElement('div')
+
 expect(queryByTestId(container, 'count-value').toBeInTheDocument()


I would've thought this line was also going to be replaced to use some of the other elements you define above that are indeed in the document.

I'd even insist in changing it merely on the ground that this is referencing a helper function in dom-testing-library, probably since this README was created based on that other library's README.

I am sorry! Great catch!

smacpherson64 · 2018-07-18T15:05:33Z

@gnapse again, Great catch!

Do the updates match what you were desiring?

gnapse

LGTM!

gnapse · 2018-07-19T14:13:15Z

Hmmm, merging this PR did not create a new release in npm, even though in the releases page it seems like something did happen. The latest two releases appear but just as tags, not as full features releases. And indeed, in npm the two new releases do not appear. It's stuck at v1.8.1.

I sense this has something to do with the recent tokens invalidations in npmjs.com, but I'm not sure how to update semantic-release's token. Care to chime in @kentcdodds?

kentcdodds · 2018-07-19T14:15:47Z

Oh yes, I manually generated a new token on npm and updated the NPM_TOKEN variable on all my Travis jobs. Took a little bit of time (I have quite a few packages) but wasn't too much of a challenge. Let me know if you need help.

There was an issue with a minor release, so this manual-releases.md change is to release a new minor version. Reference: #40

gnapse · 2018-07-19T14:25:05Z

Thanks for your help @kentcdodds! Yes, that worked. Just needed the guidance on where to change the token. We have a v1.10.0 release now 🎉.

smacpherson64 added 7 commits July 14, 2018 16:20

#34: Added test for toBeInTheDocument

ea87915

#34: Added toBeInTheDocument functionality

ff4875e

#34: Added deprecate test and functionality

ab1a19f

#34: Updated toBeInTheDOM tests to hide console.warn for clarity

a9e3eb6

#34: Added deprecated notice to toBeInTheDOM

c151c49

#34: Updated types (deprecated notice and toBeInTheDocument)

9eaaaff

#34: Updated documentation

d4d230d

smacpherson64 requested review from gnapse and jgoz July 14, 2018 22:07

jgoz requested changes Jul 14, 2018

View reviewed changes

#34: Simplified deprecate util function

c4961a5

jgoz approved these changes Jul 15, 2018

View reviewed changes

#34: Fixed grammar error

a30bfaa

smacpherson64 added 2 commits July 15, 2018 12:33

#34: Added document validation

6047eb3

#34: Cleaned up tests

0018069

#34: Updated test message for consistency

f304c07

gnapse reviewed Jul 16, 2018

View reviewed changes

smacpherson64 added 2 commits July 17, 2018 15:23

#34: Added null and undefined tests

9af1d22

#34: Updated README.md with better examples and clearer notes

a7e78e5

gnapse reviewed Jul 18, 2018

View reviewed changes

#34: Improved element selector in documentation

45740d5

gnapse approved these changes Jul 18, 2018

View reviewed changes

Merge branch 'master' into pr/depreciate-to-be-in-the-dom

2e7e70a

gnapse merged commit 3601efd into testing-library:master Jul 18, 2018

kentcdodds mentioned this pull request Jul 18, 2018

Update docs to remove toBeInTheDOM testing-library/react-testing-library#140

Closed

kentcdodds mentioned this pull request Jul 18, 2018

Update docs to remove toBeInTheDOM testing-library/dom-testing-library#75

Closed

gnapse added a commit that referenced this pull request Jul 19, 2018

feat(release): manually release a minor version

58ca030

There was an issue with a minor release, so this manual-releases.md change is to release a new minor version. Reference: #40

smacpherson64 mentioned this pull request Jul 19, 2018

Deprecate toBeInTheDOM in favor of toContainElement #34

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Depreciate toBeInTheDOM with replacement #40

feat: Depreciate toBeInTheDOM with replacement #40

smacpherson64 commented Jul 14, 2018

jgoz left a comment

jgoz Jul 14, 2018

smacpherson64 Jul 14, 2018 •

edited

Loading

jgoz Jul 14, 2018

jgoz Jul 14, 2018

smacpherson64 Jul 14, 2018

jgoz Jul 15, 2018

smacpherson64 commented Jul 14, 2018

jgoz left a comment

jgoz Jul 15, 2018

smacpherson64 Jul 15, 2018

smacpherson64 commented Jul 15, 2018 •

edited

Loading

smacpherson64 commented Jul 15, 2018 •

edited

Loading

gnapse Jul 16, 2018 •

edited

Loading

smacpherson64 Jul 17, 2018

smacpherson64 commented Jul 17, 2018 •

edited

Loading

gnapse Jul 18, 2018

smacpherson64 Jul 18, 2018

smacpherson64 commented Jul 18, 2018

gnapse left a comment

gnapse commented Jul 19, 2018

kentcdodds commented Jul 19, 2018

gnapse commented Jul 19, 2018

feat: Depreciate toBeInTheDOM with replacement #40

feat: Depreciate toBeInTheDOM with replacement #40

Conversation

smacpherson64 commented Jul 14, 2018

jgoz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smacpherson64 Jul 14, 2018 • edited Loading

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

smacpherson64 commented Jul 14, 2018

jgoz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smacpherson64 commented Jul 15, 2018 • edited Loading

TDLR

Details

Test that Caused Error

Error

smacpherson64 commented Jul 15, 2018 • edited Loading

gnapse Jul 16, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smacpherson64 commented Jul 17, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smacpherson64 commented Jul 18, 2018

gnapse left a comment

Choose a reason for hiding this comment

gnapse commented Jul 19, 2018

kentcdodds commented Jul 19, 2018

gnapse commented Jul 19, 2018

smacpherson64 Jul 14, 2018 •

edited

Loading

smacpherson64 commented Jul 15, 2018 •

edited

Loading

smacpherson64 commented Jul 15, 2018 •

edited

Loading

gnapse Jul 16, 2018 •

edited

Loading

smacpherson64 commented Jul 17, 2018 •

edited

Loading