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

wrote the test case for props in async handler(new PR) #1931

Merged
merged 1 commit into from Apr 7, 2019

Conversation

Projects
None yet
2 participants
@k3ithl1m
Copy link
Contributor

k3ithl1m commented Dec 8, 2018

Written the test case to test our props when there is an async handler #1872 @ljharb

Replaces #1930.

Closes #1872.

it('child component props should update after call to setState in async handler', () => {
const component = mount(<TestComponent />);
component.find('#parentDiv').props().onClick();
expect(component.find(TestSubComponent).props()).to.eql({ id: 'childDiv', counter: 2 });

This comment has been minimized.

Copy link
@ljharb

ljharb Dec 10, 2018

Member

so, i would expect this test to fail - because the setState has delay(100) on it.

It's impossible for the test to know when that async action is completed, unless it hardcodes a delay(100), or unless the onClick returns a promise.

This comment has been minimized.

Copy link
@k3ithl1m

k3ithl1m Dec 10, 2018

Author Contributor

Yes. It will fail. I can work on trying to fix the bug too. Does the test look right though?

This comment has been minimized.

Copy link
@ljharb

ljharb Dec 10, 2018

Member

No, to clarify - the test itself can't possibly pass. It's not possible for a test to magically wait for all async actions to be completed.

This comment has been minimized.

Copy link
@k3ithl1m

k3ithl1m Dec 11, 2018

Author Contributor

Ah okay. That makes sense. I was working on it last night and am trying to figure out.

I thought of setting a
‘Const promise = component.find(‘parentDiv’).props().onClick()’
And then promise.resolve with the expect in it. But it doesnt seem to work. I was thinking that it might be a reasonable approach because the handleClick returns a promise.

Other than that, I assume that having a delay(100) is not the best way to approach this problem.

What’s your thought?

This comment has been minimized.

Copy link
@ljharb

ljharb Dec 12, 2018

Member

I'm starting to think there's not actually a bug here, and that the original issue is invalid.

This comment has been minimized.

Copy link
@ljharb

ljharb Dec 12, 2018

Member

ok actually, the original issue in #1872 is still a bug - i think the test case needs a bit of tweaking to more closely match that (without using await or async functions)

This comment has been minimized.

Copy link
@k3ithl1m

k3ithl1m Dec 12, 2018

Author Contributor

I'm sorry but how should I go around writing this test? Or more, how am I able to know that I'm writing the test correctly?

@k3ithl1m
Copy link
Contributor Author

k3ithl1m left a comment

I rewrote this line and it seems to work. But I did some pulling and stuff and all of a sudden there was another commit with a lot of different files.

@ljharb ljharb force-pushed the k3ithl1m:testForAsyncHandler branch from 54c2d9f to 3c6f3f7 Dec 12, 2018

@ljharb

ljharb approved these changes Apr 7, 2019

Copy link
Member

ljharb left a comment

Thanks!

@ljharb ljharb force-pushed the k3ithl1m:testForAsyncHandler branch from 3c6f3f7 to 1f7d08f Apr 7, 2019

@ljharb ljharb merged commit 1f7d08f into airbnb:master Apr 7, 2019

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage remained the same at 91.502%
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.