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

Fix Test timeout #100

Merged
merged 3 commits into from
Sep 27, 2023
Merged

Fix Test timeout #100

merged 3 commits into from
Sep 27, 2023

Conversation

ChristopherChudzicki
Copy link
Contributor

@ChristopherChudzicki ChristopherChudzicki commented Sep 25, 2023

What are the relevant tickets?

Closes #98

Description (What does it do?)

This PR changes one test that was being problematic.

How can this be has this been tested?

  1. I ran the original test in a .each() block, repeated it 100 times, on both CI and locally. See https://github.com/mitodl/mit-open/actions/runs/6301758705/job/17107497440?pr=100
    • locally: that test file took ~130 seconds
    • on CI: 4 ❌, 96 ✅
  2. I removed an unnecessary findByRole query, and repeated (1), running 100 tests. See https://github.com/mitodl/mit-open/actions/runs/6301882578
    • locally: that test file took ~55 seconds
    • on CI: 100 ✅

So, the test is a lot faster now.

Additional Context

Testing Library's *byRole queiries are their recommended query because they provide a number of accessibility checks, but they apparently slower than related queries that do not make similar accessibility checks (e.g., *byText*). See for example:

But the performance is getting better: The unreleased branch of jest-environment-jsdom uses JSDom v22 (we're on v20). Trying that locally brought 100 repetitions down to undo 40 seconds.

@ChristopherChudzicki ChristopherChudzicki changed the title run test 100 times to check timeout issue on CI. Test timeout Sep 25, 2023
@codecov
Copy link

codecov bot commented Sep 25, 2023

Codecov Report

Merging #100 (2ce8137) into main (089b5c3) will decrease coverage by 0.02%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main     #100      +/-   ##
==========================================
- Coverage   78.02%   78.01%   -0.02%     
==========================================
  Files         323      323              
  Lines       14357    14357              
  Branches     2320     2320              
==========================================
- Hits        11202    11200       -2     
- Misses       2862     2863       +1     
- Partials      293      294       +1     

see 4 files with indirect coverage changes

Comment on lines +109 to +110
expect(reorderButton).toHaveAccessibleName("Done ordering")
await user.click(reorderButton)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The button is actually the same element, just it's text changes. So there's no need to re-query the DOM to find it.

@ChristopherChudzicki ChristopherChudzicki added the Needs Review An open Pull Request that is ready for review label Sep 25, 2023
@ChristopherChudzicki ChristopherChudzicki marked this pull request as ready for review September 25, 2023 17:07
@ChristopherChudzicki ChristopherChudzicki changed the title Test timeout Fix Test timeout Sep 25, 2023
@abeglova abeglova self-assigned this Sep 26, 2023
@abeglova abeglova added Waiting on author and removed Needs Review An open Pull Request that is ready for review labels Sep 26, 2023
@ChristopherChudzicki ChristopherChudzicki merged commit 506cd3d into main Sep 27, 2023
@rhysyngsun rhysyngsun deleted the cc/test-timeout branch February 7, 2025 20:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Intermittent JS test timeouts
2 participants