Skip to content

Conversation

mpeyper
Copy link
Member

@mpeyper mpeyper commented Jun 29, 2021

What:

Refactor tests to reduce duplication for different renderers.

Why:

Our test suite has lots of duplication, running the same (or almost the same) test against the dom, native and server renderers.

How:

Added a test utility that runs tests against groups of renderers.

Checklist:

  • Tests
  • Ready to be merged

For some reason, the current tests were not having eslint warnings appear. When moving them into src/__tests__/, the warnings appeared. This actually highlighted an issue with the type of the cleanup export which is is an async function, but wasn't returning a Promise in the types. I've included updating them in this PR so we must remember to merge this as a fix rather that the test commit.

@mpeyper mpeyper requested a review from joshuaellis June 29, 2021 13:45
@codecov
Copy link

codecov bot commented Jun 29, 2021

Codecov Report

Merging #643 (49cf925) into main (eff2ca6) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##              main      #643   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           15        15           
  Lines          235       235           
  Branches        29        33    +4     
=========================================
  Hits           235       235           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update eff2ca6...49cf925. Read the comment docs.

@@ -0,0 +1,258 @@
import { useState, useRef, useEffect } from 'react'

describe('async hook tests', () => {
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 wouldn't mind having another go at this test file sometime (not in this change). Currently, it the longest running test by far, clocking almost 20s on my macbook to run all the tests for all the renderers. We had problems in the past making the delays too small, but that was before the refactoring of the asyncUtils which I think has made things more reliable.

Copy link
Member

@joshuaellis joshuaellis left a comment

Choose a reason for hiding this comment

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

It's great to see us loose so much duplication code! Also this way for managing the test suites is also super interesting to see 🚀

@mpeyper mpeyper merged commit c7a2e97 into main Jun 30, 2021
@mpeyper mpeyper deleted the deduping-tests branch June 30, 2021 21:07
@github-actions
Copy link

🎉 This PR is included in version 7.0.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants