Dom-testing-library: Add callback to waitForDomChange to support sync updates

[mlrawlings]

Describe the feature you'd like:

Given the following scenario:

fireEvent.click(getByRole('button'));
await waitForDomChange();

If the DOM changes synchronously due to the click, waitForDomChange will never resolve because it missed the change. Many component libraries batch DOM updates, but if a component manually updates the DOM in response to an event (or is wrapping a vanilla js library), the change may be synchronous. But this is an implementation detail.

Every other wait* api uses a polling callback that will detect sync changes as well async changes. I'd like an API that can detect some unknown DOM change due to an action regardless of whether the change is sync or async.

Suggested implementation:

Support passing an optional action callback to the waitForDomChange function:

await waitForDomChange(() => {
  fireEvent.click(getByRole('button'));
});

Implementation:

async function __oldWaitForDomChange(/* ... */) {
    /* ... */
}

async function waitForDomChange(options, fn) {
    if (typeof options === "function") {
        fn = options;
        options = undefined;
    }
    const change = __oldWaitForDomChange(options);
    if (fn) await fn();
    await change;
}

Describe alternatives you've considered:

Begin watching for the change before the action:

const change = waitForDomChange();
fireEvent.click(getByRole('button'));
await change;

Wait for a known change:

fireEvent.click(getByRole('button'));
await wait(() => expect(getByRole('button')).toHaveAttribute("aria-expanded", "true")) ;

Teachability, Documentation, Adoption, Migration Strategy:

What do you think? Is this a good approach?

[alexkrolick]

Wait for a known change

This is generally the recommended approach - use explicit queries. Using wait or waitForDomChange without a condition makes for flaky tests.

[kentcdodds]

Yup, what Alex said 👍

[alexkrolick]

Also you can use find queries (like findByText) to get async-by-default queries that will also work in the synchronous case.

fireEvent.click(getByRole('button'));
expect(await findByRole('button')).toHaveAttribute("aria-expanded", "true");

[mlrawlings]

For the most part I agree. So you have some context, specifically where I felt the need for this was in a given ... when ... then setup where the assertion is meant to be separate from the action. But I do want to know that some effect from the action has taken place before moving on...

describe('given the menu is in the default state', () => {
    beforeEach(async() => {
        component = await render(template, { text: 'text', items: mock.twoItems });
        button = component.getByRole('button');
    });

    describe('when the button is clicked once', () => {
        beforeEach(async() => {
            await waitForDomChange(() => fireEvent.click(button));
        });

        it('then it expands', async() => {
            expect(button).to.have.attr('aria-expanded', 'true');
        });

        it('then it emits the expand event', () => {
            expect(component.emitted('menu-expand')).has.length(1);
        });

        describe('when it is clicked again', () => {
            beforeEach(async() => {
                await waitForDomChange(() => fireEvent.click(button));
            });

            it('then it collapses', async() => {
                expect(button).to.have.attr('aria-expanded', 'false');
            });

            it('then it emits the collapse event', () => {
                expect(component.emitted('menu-collapse')).has.length(1);
            });
        });
    });
});

I could change this to something redundant like this, but it feels weird.

beforeEach(async() => {
    fireEvent.click(button);
    await wait(() => expect(button).to.have.attr('aria-expanded', 'true'));
});

it('then it expands', async() => {
    expect(button).to.have.attr('aria-expanded', 'true');
});

And currently, the effects of the click for this component are sync. So this would work fine:

beforeEach(() => {
    fireEvent.click(button);
});

it('then it expands', async() => {
    expect(button).to.have.attr('aria-expanded', 'true');
});

However this is only the case because the aria-expanded attribute is controlled by a utility that synchronously updates the DOM. We're probably going to have this attribute controlled by state in the near future and I'd like the test to not break when that happens.

[kentcdodds]

I see. Here's how I would write this test:

test('when the the button is clicked it expands and emits the expand event', async () => {
  const component = await render(template, {text: 'text', items: mock.twoItems})
  const button = component.getByRole('button')
  fireEvent.click(button)
  await waitForDomChange(() => expect(button).to.have.attr('aria-expanded', 'true'))
  expect(component.emitted('menu-expand')).has.length(1)
})

test('when the button is clicked twice, it collapses and emits the collapse event', async () => {
  const component = await render(template, {text: 'text', items: mock.twoItems})
  const button = component.getByRole('button')
  fireEvent.click(button)
  fireEvent.click(button)
  await waitForDomChange(() => expect(button).to.have.attr('aria-expanded', 'false'))
  expect(component.emitted('menu-collapse')).has.length(1)
})

I think this kind of test is much more straightforward. There aren't any mutable variables to keep track of. I talk a little bit about it here: https://www.briefs.fm/3-minutes-with-kent/27

If you really want to reduce duplication then you could do it like this:

async function setup() {
  const component = await render(template, {text: 'text', items: mock.twoItems})
  const button = component.getByRole('button')
  return {component, button}
}

test('when the the button is clicked it expands and emits the expand event', async () => {
  const {component, button} = await setup()
  fireEvent.click(button)
  await waitForDomChange(() => expect(button).to.have.attr('aria-expanded', 'true'))
  expect(component.emitted('menu-expand')).has.length(1)
})

test('when the button is clicked twice, it collapses and emits the collapse event', async () => {
  const {component, button} = await setup()
  fireEvent.click(button)
  fireEvent.click(button)
  await waitForDomChange(() => expect(button).to.have.attr('aria-expanded', 'false'))
  expect(component.emitted('menu-collapse')).has.length(1)
})

That's what I do in my tests and it works out really well :)

[alexkrolick]

• It kind of seems like you are using it for "labelled expectations". As written, isn't the beforeEach() causing a click on the button between each it in the describe()? Maybe you meant beforeAll? (Also, sharing state across it/test can be pretty risky if you forget to clean up the DOM, which is commonly done using global afterEach hooks).

• I agree with Kent's suggestion on how to structure the tests, but if you really want to keep the existing flow you can move the await wait() into each it().

// inside the describe()
// fireEvent.click(button) already happened
it('then it expands', async () => {
  await wait(() => expect(button).to.have.attr('aria-expanded', 'true'));
});

[mlrawlings]

Here's how I would write this test: [...]

If I were writing it from scratch, I'd do the same. I'm helping a team rewrite their tests to use testing-library and all their tests are using this gherkin-style. Perhaps there's value in simplifying the structure at this point as well, but we were going for leaving the same tests intact with a minimal diff. Though I'm not sure they'd agree... not everyone does.

if you really want to keep the existing flow you can move the await wait() into each it()

I initially did that, but it means that each test needs to wait, which would have been fine, but the inner describe would also have to wait for... something. I guess it could wait for something that's been already asserted, but that's back to a redundant solution.

Using wait or waitForDomChange without a condition makes for flaky tests.

await waitForDomChange(() => expect(button).to.have.attr('aria-expanded', 'true'))

☝️ This doesn't work, does it? waitForDomChange doesn't accept a callback/condition. I'm kinda curious why this API was introduced (or wait()) and how you envision it being used.

I agree it may not be _ideal_, but waitForDomChange() seems like a slight step up from wait() in that you know _something_ happened, not just that a tick has passed.

I just found it not ideal that

fireEvent.click(button)
await wait()

is fine assuming the change happens in that tick (sync or async), but

fireEvent.click(button)
await waitForDomChange()

times out if the DOM change is sync.

[alexkrolick]

Why do you need to wait for the DOM change when wait will retry the expect until it passes? https://testing-library.com/docs/dom-testing-library/api-async#wait

wait is not sleep