Skip to content
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

chore(tests): Migrate from Enzyme to RTL (1st batch) #1535

Merged
merged 9 commits into from
Sep 23, 2024

Conversation

loonskai
Copy link
Contributor

@loonskai loonskai commented Sep 6, 2024

Description

Tests with Enzyme migrated: 23/61

This PR is a part of Box-wide migration to the newer React v18.

  • The most of the scenarios were rewritten with consideration of RTL practices (thanks @nmkedziora for pointing this out). Some components required fireEvent executed on top of specific selectors instead of recommended userEvent. Some of these tradeoffs could be avoided (possibly) but would extend the timelines of this PR. I suggest improve these test iteratively as we work on them.
  • Semantic selectors were used in most cases. data-testid attributes were added mostly for container elements to quickly validate if container is rendered.
  • This PR doesn't introduce any runtime changes (except data-testid changes mentioned above)

This PR will be followed with the next steps:

Screenshots

image

@loonskai loonskai self-assigned this Sep 6, 2024
@loonskai loonskai marked this pull request as ready for review September 17, 2024 15:53
@loonskai loonskai requested a review from a team as a code owner September 17, 2024 15:53
@loonskai loonskai changed the title chore(tests): Migrate from Enzyme to RTL chore(tests): Migrate from Enzyme to RTL (1st batch) Sep 17, 2024
@rafalmaksymiuk
Copy link

This is a massive PR - tremendous work but hard to review. I know it is difficult to push each PR separately, but is way easier to meaningfully review so maybe split more next time :)?

Another thought - or rather question. I do know that EUA uses special tooling based on RTL. Don't remember details, but it integrates Redux somehow. Do you know why it was needed there and if it might be here?

rafalmaksymiuk
rafalmaksymiuk previously approved these changes Sep 18, 2024
@nmkedziora
Copy link

nmkedziora commented Sep 18, 2024

This is a massive PR - tremendous work but hard to review. I know it is difficult to push each PR separately, but is way easier to meaningfully review so maybe split more next time :)?

+1 for me.
Amazing job and great effort 🎉

Splitting the PR by feature or folder would make a significant difference. It would not only make it easier to review but also easier for you to work on the changes (in terms of time and capacity) and quicker to merge bit by bit instead of blocking the merge for the entire PR due to comments on a single file.

expect(screen.getByLabelText('Breadcrumb')).toBeInTheDocument();
expect(screen.getAllByRole('button')).toHaveLength(3);
expect(screen.getAllByRole('button').at(0).textContent).toContain('test.zip');
expect(screen.getAllByRole('button').at(1).textContent).toContain('test');

Choose a reason for hiding this comment

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

Nit: you could do screen.getAllByRole('button') only once:

Suggested change
expect(screen.getAllByRole('button').at(1).textContent).toContain('test');
const buttons = screen.getAllByRole('button')
expect(buttons.at(1).textContent).toContain('test');

@loonskai loonskai merged commit 9a2ed15 into box:master Sep 23, 2024
3 checks passed
@loonskai loonskai deleted the WEBAPP-25308 branch September 23, 2024 08:55
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.

4 participants