Skip to content

Convert enzyme tests to React Testing Library with help from Claude Code#1974

Open
cnathe wants to merge 1 commit intodevelopfrom
fb_claudeEnzymeToRTL
Open

Convert enzyme tests to React Testing Library with help from Claude Code#1974
cnathe wants to merge 1 commit intodevelopfrom
fb_claudeEnzymeToRTL

Conversation

@cnathe
Copy link
Copy Markdown
Contributor

@cnathe cnathe commented Apr 6, 2026

Rationale

Major step towards finishing our conversion of our enzyme jest tests to RTL jest tests, using Claude Code for the conversion.

Related Pull Requests

@cnathe cnathe self-assigned this Apr 6, 2026
@cnathe cnathe requested a review from labkey-alan April 6, 2026 14:39
Copy link
Copy Markdown
Contributor

@labkey-alan labkey-alan left a comment

Choose a reason for hiding this comment

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

Overall the conversions look correct, however, in many cases I've found that Claude replaced await waitForLifeCycle() with await act(async () => {});, and I think we should probably be waiting for something specific in order to prevent flaky tests from being an issue. In at least one test suite I was able to drop the await act entirely with no problem.

);

expect(document.querySelector('.fa-spinner')).not.toBeNull();
await act(async () => {});
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

There are 4 lines that look like this, I believe all are meant to replace calls to waitForLifecycle, they should be replaced with something more specific.

data: fromJS({ value: '2022-02-12 11:58:54.385', formattedValue: '2022-02-12' }),
};

describe('ExpirationDateColumnRenderer', () => {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This test emits an error Warning: validateDOMNesting(...): <td> cannot appear as a child of <div>, but does not fail. I think we should consider cleaning up the error in order to reduce test noise. I think in order to clean up the error you'll need to create a wrapper in the test for all cases except not tablecell. Something that looks like:

const TestWrapper = props => (
    <table>
        <tbody>
            <tr>
                <ExpirationDateColumnRenderer {...props} />
            </tr>
        </tbody>
    </table>
);

<UserLink userDisplayValue="Test display" />,
{ serverContext: { user: TEST_USER_APP_ADMIN } }
);
await act(async () => {});
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The test contains six different lines that look like this, but if you delete them, and remove the act import, the test still passes.

}
);

await act(async () => {});
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

There are several of these lines in this test suite. I think the test should probably be waiting for something specific or we're likely to have flaky tests.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants