Pr/cleanup renderhooks #106
Conversation
Codecov Report
@@ Coverage Diff @@
## master #106 +/- ##
=====================================
Coverage 100% 100%
=====================================
Files 1 1
Lines 39 43 +4
Branches 3 3
=====================================
+ Hits 39 43 +4
Continue to review full report at Codecov.
|
| @@ -51,6 +53,11 @@ function resultContainer() { | |||
| } | |||
| } | |||
|
|
|||
| function cleanup() { | |||
| unmounts.forEach((unmount) => unmount()) | |||
| unmounts.length = 0 | |||
There was a problem hiding this comment.
is there significant difference between this and unmounts = []? That feels a bit clearer what it's doing to me, but I'm not sure if there are some other hidden benefits to this approach?
There was a problem hiding this comment.
I was thinking to use either mutable or immutable approach. Personally, i prefer the immutable way as i suggested in the issue.
let unmounts = [];
unmounts = [...unmounts, unmount];
unmounts = [];
but i saw the way that you are using the resolvers
const resolvers = [];
resolvers.push(resolver);
and i tried to follow the same pattern for consistency (mutable). I don't have a strong opinion on this, it's up to you.
| @@ -73,6 +80,14 @@ function renderHook(callback, { initialProps, wrapper } = {}) { | |||
| }) | |||
| const { unmount, update } = testRenderer | |||
|
|
|||
| function unmountHook() { | |||
There was a problem hiding this comment.
what happens if someone calls this then then cleanup? does the second unmount fail? should unmountHook also remove the callback from unmounts (making this line no longer necessary at all)
There was a problem hiding this comment.
I see there is a test around this case. I personally feel that having unmountHook remove the callback from the unmounts array is bit clearer to what is happening rather than relying on the behaviour of unmount, but I don't care enough to block this for it.
There was a problem hiding this comment.
I agree with you, i will fix it.
| @@ -0,0 +1 @@ | |||
| afterEach(require('./src').cleanup) | |||
There was a problem hiding this comment.
I think this should be requireing ./lib so that it picks up the compiled version after publishing.
|
Hi @apostolidhs, Thanks for taking the time to put this together. There was still some discussion in #76 about whether this should be called |
|
I just read the description properly and noticed your question about a global option. Exploring that a bit, would it be enabled by default and they can disable globally if they have performance issues or is it disabled by default and they have to opt in to use it? We could enable it by default if they use the I also think it would take an awful lot of tests before someone actually saw an issue from not calling |
|
Just another thought, could we auto clear on a Would this result in less cases where anyone actually needs to call Would this result in more strange failures because something throwing on unmount would fail the next test? |
|
My comment about |
|
@mpeyper Can you take a look again? I committed the changes as a fixup.
|
|
Sorry, I haven't had time to looks at this again yet (end of financial year is Oz and I work in financial services). I'm planning to look in the next few days. |
|
@apostolidhs Those changes look good. I found them here, but they do not appear to be in this branch (I did notice the branch they are on is missing the The only bit I'm not sure on is where you've put the documentation (in the README). It would be great to get it into the docs site. Perhaps more or less what you have in the setup page and then some details on the I can tidy all that up after we merge this if you prefer too. Just let me know. |
|
I'm not sure where this is at right now, but react-testing-library has a PR open around auto cleanup, which might be something we want to explore instead of/in addition to this. |
|
I'm going to close this as it appears to have gone stale (no updates in months and 4/6 changed files not have conflict. I'm still very much in favour of this change, so if anyone wants to pick it up from here then I'm happy to continue. Alternatively, if you want to start fresh, that's cool too. |
What:
Added global cleanup
Why:
We should unmount all the rendered hooks after each test so we can avoid memory leaks, flaky tests and fully test the hooks on the entire lifecycle.
How:
We keep the functions that unmount of the rendered hooks in a list and export a
cleanupfunction that iterates the list and unmounts the hooks.Checklist:
I don't know if we should add a global option on the library that enables the cleanup feature. If a client runs the tests without calling the cleanup function, then we will collect a list of the
unmountfunctions. This will may cause performance issues in a huge amount of tests.Related issue #76