Skip to content

Conversation

rolandszoke
Copy link
Member

No description provided.

@rolandszoke rolandszoke requested a review from solkimicreb March 16, 2020 10:43
Copy link
Collaborator

@solkimicreb solkimicreb left a comment

Choose a reason for hiding this comment

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

Looks nice. I added a question but no changes are required. I am merging after I get my answer 🙂


const clearIntervalSpy = sinon.spy(global, 'clearInterval');

afterAll(cleanup);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Out of curiosity? Why was this afterAll split required?

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'm not sure, but @testing-library/dom changed, and If you do not add the cleanup directly to the afterAll or afterEach, but run it in a function (it does not matter if it is an arrow function or not) the cleanup will lose the scope of the document so it is unable to find the body and clean it up, and the function breaks.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ideally, we could have our tests isolated so we would not need to use cleanup at all. Maybe I should start working on it.

@rolandszoke rolandszoke requested a review from solkimicreb March 16, 2020 16:13
@coveralls
Copy link

coveralls commented Mar 17, 2020

Coverage Status

Coverage remained the same at 96.029% when pulling d9e245a on update-deps_2020_03_16 into 8830963 on master.

Copy link
Collaborator

@solkimicreb solkimicreb left a comment

Choose a reason for hiding this comment

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

I got it and also tried it out. I also have no idea why it does that. Could you add a comment on top of these separated afterAll/afterEach cases that explains why they are necessary and warns future devs to do not merge them into single functions? Thanks!

@rolandszoke rolandszoke requested a review from solkimicreb March 17, 2020 10:44
Copy link
Collaborator

@solkimicreb solkimicreb left a comment

Choose a reason for hiding this comment

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

Looks good!

@solkimicreb solkimicreb merged commit d0a0297 into master Mar 17, 2020
@solkimicreb solkimicreb deleted the update-deps_2020_03_16 branch March 30, 2020 07:43
@rolandszoke
Copy link
Member Author

🎉 This PR is included in version 6.2.0 🎉

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.

3 participants