Skip to content
This repository has been archived by the owner on Oct 1, 2024. It is now read-only.

Fix nested wrappers bug in react-testing #697

Merged
merged 1 commit into from
May 13, 2019
Merged

Conversation

cartogram
Copy link
Contributor

@cartogram cartogram commented May 10, 2019

This PR fixes a bug in react-testing where an unresolved promise required an additional act() around the state change.

@cartogram cartogram requested review from lemonmade and marutypes May 10, 2019 19:42
@cartogram cartogram force-pushed the fix-nested-wrappers branch 2 times, most recently from a49fc3f to 05e95e6 Compare May 10, 2019 19:47
@cartogram cartogram force-pushed the fix-nested-wrappers branch from 05e95e6 to 1b476c1 Compare May 10, 2019 19:49
@@ -107,6 +107,8 @@ export class Root<Props> {
});

if (isPromise(result)) {
updateWrapper();
Copy link
Member

Choose a reason for hiding this comment

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

Hmm, I'm not sure I understand why you would do it here, also. if the result is a promise, there shouldn't be a reason to update until after the act block has finished, is there?

Copy link
Contributor

@marutypes marutypes May 11, 2019

Choose a reason for hiding this comment

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

If the function you triggered looks something like

async function doASubmit() { 
   setSomeLoadingState(true);
   await sendSomeRequest();
   setSomeLoadingState(false);
}

Then after synchronous execution pauses on the await there has been a state update, if we don't call updateWrapper then any test for loading states in cases will fail.

Copy link
Member

Choose a reason for hiding this comment

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

For this to make sense, setSomeLoadingState has to run synchronously. I don't think that's guaranteed for async functions?

Copy link
Member

Choose a reason for hiding this comment

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

Resolved in Slack, we agreed that this is probably the expected behaviour, and if they need to wait until any other update is finished and then update the tree, they can do so with act

@cartogram
Copy link
Contributor Author

cartogram commented May 13, 2019

@lemonmade @TheMallen updated the PR to do 2 things:
1) made the update option for act is an enum of Pending, Complete or Never. The default is still Complete as before, but now we can offer somewhere for users to configure this from their tests.
2) expose the update method publicly on the root so that users can call the wrapper.update() after a loading state is triggered and then assert on the updated UI.

Update: I removed those and still going with what I had initially.

@cartogram cartogram force-pushed the fix-nested-wrappers branch from c230c46 to 1b476c1 Compare May 13, 2019 18:18
@@ -107,6 +107,8 @@ export class Root<Props> {
});

if (isPromise(result)) {
updateWrapper();
Copy link
Member

Choose a reason for hiding this comment

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

Resolved in Slack, we agreed that this is probably the expected behaviour, and if they need to wait until any other update is finished and then update the tree, they can do so with act

@cartogram cartogram merged commit 13de09e into master May 13, 2019
@michenly michenly deleted the fix-nested-wrappers branch May 14, 2019 14:42
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants