Skip to content

feat(wait): wait will now also run your callback on DOM changes #415

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

Merged
merged 7 commits into from
Mar 4, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -45,8 +45,7 @@
"@types/testing-library__dom": "^6.12.1",
"aria-query": "^4.0.2",
"dom-accessibility-api": "^0.3.0",
"pretty-format": "^25.1.0",
"wait-for-expect": "^3.0.2"
"pretty-format": "^25.1.0"
},
"devDependencies": {
"@testing-library/jest-dom": "^5.1.1",
Expand All @@ -61,7 +60,8 @@
"rules": {
"import/prefer-default-export": "off",
"import/no-unassigned-import": "off",
"import/no-useless-path-segments": "off"
"import/no-useless-path-segments": "off",
"no-console": "off"
}
},
"eslintIgnore": [
Expand Down
52 changes: 52 additions & 0 deletions src/__tests__/fake-timers.js
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ jest.useFakeTimers()
jest.resetModules()

const {
wait,
waitForElement,
waitForDomChange,
waitForElementToBeRemoved,
Expand All @@ -42,6 +43,15 @@ test('waitForElementToBeRemoved: times out after 4500ms by default', () => {
return promise
})

test('wait: can time out', async () => {
const promise = wait(() => {
// eslint-disable-next-line no-throw-literal
throw undefined
})
jest.advanceTimersByTime(4600)
await expect(promise).rejects.toThrow(/timed out/i)
})

test('waitForElement: can time out', async () => {
const promise = waitForElement(() => {})
jest.advanceTimersByTime(4600)
Expand Down Expand Up @@ -85,3 +95,45 @@ test('waitForDomChange: can specify our own timeout time', async () => {
// timed out
await expect(promise).rejects.toThrow(/timed out/i)
})

test('wait: ensures the interval is greater than 0', async () => {
// Arrange
const spy = jest.fn()
spy.mockImplementationOnce(() => {
throw new Error('first time does not work')
})
const promise = wait(spy, {interval: 0})
expect(spy).toHaveBeenCalledTimes(1)
spy.mockClear()

// Act
// this line will throw an error if wait does not make the interval 1 instead of 0
// which is why it does that!
jest.advanceTimersByTime(0)

// Assert
expect(spy).toHaveBeenCalledTimes(0)
spy.mockImplementationOnce(() => 'second time does work')

// Act
jest.advanceTimersByTime(1)
await promise

// Assert
expect(spy).toHaveBeenCalledTimes(1)
})

test('wait: times out if it runs out of attempts', () => {
const spy = jest.fn(() => {
throw new Error('example error')
})
// there's a bug with this rule here...
// eslint-disable-next-line jest/valid-expect
const promise = expect(
wait(spy, {interval: 1, timeout: 3}),
).rejects.toThrowErrorMatchingInlineSnapshot(`"example error"`)
jest.advanceTimersByTime(1)
jest.advanceTimersByTime(1)
jest.advanceTimersByTime(1)
return promise
})
2 changes: 0 additions & 2 deletions src/__tests__/pretty-dom.js
Original file line number Diff line number Diff line change
Expand Up @@ -77,5 +77,3 @@ describe('prettyDOM fails with first parameter without outerHTML field', () => {
)
})
})

/* eslint no-console:0 */
2 changes: 0 additions & 2 deletions src/__tests__/role-helpers.js
Original file line number Diff line number Diff line change
Expand Up @@ -184,5 +184,3 @@ test.each([

expect(isInaccessible(container.querySelector('button'))).toBe(expected)
})

/* eslint no-console:0 */
2 changes: 0 additions & 2 deletions src/__tests__/screen.js
Original file line number Diff line number Diff line change
Expand Up @@ -61,5 +61,3 @@ test('exposes debug method', () => {
`)
console.log.mockClear()
})

/* eslint no-console:0 */
8 changes: 8 additions & 0 deletions src/__tests__/wait-for-dom-change.js
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ beforeEach(() => {
jest.useRealTimers()
jest.resetModules()
waitForDomChange = importModule()
console.warn.mockClear()
})

test('waits for the dom to change in the document', async () => {
Expand All @@ -34,6 +35,13 @@ test('waits for the dom to change in the document', async () => {
},
]
`)
expect(console.warn.mock.calls).toMatchInlineSnapshot(`
Array [
Array [
"\`waitForDomChange\` has been deprecated. Use \`wait\` instead: https://testing-library.com/docs/dom-testing-library/api-async#wait.",
],
]
`)
})

test('waits for the dom to change in a specified container', async () => {
Expand Down
21 changes: 18 additions & 3 deletions src/__tests__/wait-for-element-to-be-removed.js
Original file line number Diff line number Diff line change
Expand Up @@ -49,23 +49,23 @@ test('requires a function as the first parameter', () => {
return expect(
waitForElementToBeRemoved(),
).rejects.toThrowErrorMatchingInlineSnapshot(
`"waitForElementToBeRemoved requires a function as the first parameter"`,
`"waitForElementToBeRemoved requires a callback as the first parameter"`,
)
})

test('requires an element to exist first', () => {
return expect(
waitForElementToBeRemoved(() => null),
).rejects.toThrowErrorMatchingInlineSnapshot(
`"The callback function which was passed did not return an element or non-empty array of elements. waitForElementToBeRemoved requires that the element(s) exist before waiting for removal."`,
`"The callback function which was passed did not return an element or non-empty array of elements. waitForElementToBeRemoved requires that the element(s) exist(s) before waiting for removal."`,
)
})

test('requires an unempty array of elements to exist first', () => {
return expect(
waitForElementToBeRemoved(() => []),
).rejects.toThrowErrorMatchingInlineSnapshot(
`"The callback function which was passed did not return an element or non-empty array of elements. waitForElementToBeRemoved requires that the element(s) exist before waiting for removal."`,
`"The callback function which was passed did not return an element or non-empty array of elements. waitForElementToBeRemoved requires that the element(s) exist(s) before waiting for removal."`,
)
})

Expand Down Expand Up @@ -117,3 +117,18 @@ test("doesn't change jest's timers value when importing the module", () => {

expect(window.setTimeout._isMockFunction).toEqual(true)
})

test('rethrows non-testing-lib errors', () => {
let throwIt = false
const div = document.createElement('div')
const error = new Error('my own error')
return expect(
waitForElementToBeRemoved(() => {
if (throwIt) {
throw error
}
throwIt = true
return div
}),
).rejects.toBe(error)
})
8 changes: 8 additions & 0 deletions src/__tests__/wait-for-element.js
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ beforeEach(() => {
jest.useRealTimers()
jest.resetModules()
waitForElement = importModule()
console.warn.mockClear()
})

test('waits for element to appear in the document', async () => {
Expand All @@ -18,6 +19,13 @@ test('waits for element to appear in the document', async () => {
setTimeout(() => rerender('<div data-testid="div" />'))
const element = await promise
expect(element).toBeInTheDocument()
expect(console.warn.mock.calls).toMatchInlineSnapshot(`
Array [
Array [
"\`waitForElement\` has been deprecated. Use a \`find*\` query (preferred: https://testing-library.com/docs/dom-testing-library/api-queries#findby) or use \`wait\` instead (it's the same API, so you can find/replace): https://testing-library.com/docs/dom-testing-library/api-async#wait",
],
]
`)
})

test('waits for element to appear in a specified container', async () => {
Expand Down
11 changes: 11 additions & 0 deletions src/__tests__/wait.js
Original file line number Diff line number Diff line change
Expand Up @@ -17,3 +17,14 @@ test('wait defaults to a noop callback', async () => {
await wait()
expect(handler).toHaveBeenCalledTimes(1)
})

test('can timeout after the given timeout time', async () => {
const error = new Error('throws every time')
const result = await wait(
() => {
throw error
},
{timeout: 8, interval: 5},
).catch(e => e)
expect(result).toBe(error)
})
6 changes: 4 additions & 2 deletions src/config.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ import {prettyDOM} from './pretty-dom'
// './queries' are query functions.
let config = {
testIdAttribute: 'data-testid',
asyncUtilTimeout: 4500,
asyncUtilTimeout: 1000,
Copy link
Member Author

Choose a reason for hiding this comment

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

Just realized I amended the first of those two changes. So this is all that first change was.

// this is to support React's async `act` function.
// forcing react-testing-library to wrap all async functions would've been
// a total nightmare (consider wrapping every findBy* query and then also
Expand All @@ -19,9 +19,11 @@ let config = {

// called when getBy* queries fail. (message, container) => Error
getElementError(message, container) {
return new Error(
const error = new Error(
[message, prettyDOM(container)].filter(Boolean).join('\n\n'),
)
error.name = 'TestingLibraryElementError'
return error
},
}

Expand Down
4 changes: 1 addition & 3 deletions src/pretty-dom.js
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ const inNode = () =>
const getMaxLength = dom =>
inCypress(dom)
? 0
: typeof process !== 'undefined' && process.env.DEBUG_PRINT_LIMIT || 7000
: (typeof process !== 'undefined' && process.env.DEBUG_PRINT_LIMIT) || 7000

const {DOMElement, DOMCollection} = prettyFormat.plugins

Expand Down Expand Up @@ -64,5 +64,3 @@ function prettyDOM(dom, maxLength, options) {
const logDOM = (...args) => console.log(prettyDOM(...args))

export {prettyDOM, logDOM}

/* eslint no-console:0 */
2 changes: 0 additions & 2 deletions src/role-helpers.js
Original file line number Diff line number Diff line change
Expand Up @@ -176,5 +176,3 @@ export {
prettyRoles,
isInaccessible,
}

/* eslint no-console:0 */
11 changes: 11 additions & 0 deletions src/wait-for-dom-change.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,11 @@ import {
} from './helpers'
import {getConfig} from './config'

let hasWarned = false

// deprecated... TODO: remove this method. People should use wait instead
// the reasoning is that waiting for just any DOM change is an implementation
// detail. People should be waiting for a specific thing to change.
Comment on lines +13 to +15
Copy link
Member

Choose a reason for hiding this comment

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

If we plan to remove this method in the next major release, should be add a warning so people know they are relying on a soon-to-be-gone method? Is it too invasive?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm kinda thinking that we should add the warning in the major version bump, and then remove it in the next one.

function waitForDomChange({
container = getDocument(),
timeout = getConfig().asyncUtilTimeout,
Expand All @@ -18,6 +23,12 @@ function waitForDomChange({
characterData: true,
},
} = {}) {
if (!hasWarned) {
hasWarned = true
console.warn(
`\`waitForDomChange\` has been deprecated. Use \`wait\` instead: https://testing-library.com/docs/dom-testing-library/api-async#wait.`,
)
}
return new Promise((resolve, reject) => {
const timer = setTimeout(onTimeout, timeout)
const observer = newMutationObserver(onMutation)
Expand Down
Loading