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

Add testAsync example with ReactTestUtils actAsync usage #77

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

TristanCacqueray
Copy link

No description provided.

@TristanCacqueray
Copy link
Author

Not sure this README is the appropriate place for such example, but as it took me quite a while to figure out this usage, and assuming this is the right strategy, perhaps bs-jest could document how it can be used to test react hook?

Copy link
Owner

@glennsl glennsl left a comment

Choose a reason for hiding this comment

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

Thanks! I don't know where else to put this, so I think this is fine. It would probably be better to use testPromise though, as shown below, if you could verify that this works?

Comment on lines +64 to +77
testAsync("actAsync", finish => {
let container = getContainer(container);
// actAsync returns a promise for all effects evaluation
actAsync(_ =>
// Convert ReactDOMRe.render to a promise
resolve(ReactDOMRe.render(effectfulComponent, container))
) |> then_(_ =>
// Call finish with assertion on final container
finish(expect(
container
->DOM.findBySelectorAndTextContent("p", "Hello async Jest")
->Belt.Option.isSome) |> toBe(true)) |> resolve
) |> ignore;
});
Copy link
Owner

Choose a reason for hiding this comment

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

It would be more appropriate to use testPromise here, I think. Then you should be able to do:

Suggested change
testAsync("actAsync", finish => {
let container = getContainer(container);
// actAsync returns a promise for all effects evaluation
actAsync(_ =>
// Convert ReactDOMRe.render to a promise
resolve(ReactDOMRe.render(effectfulComponent, container))
) |> then_(_ =>
// Call finish with assertion on final container
finish(expect(
container
->DOM.findBySelectorAndTextContent("p", "Hello async Jest")
->Belt.Option.isSome) |> toBe(true)) |> resolve
) |> ignore;
});
testPromise("actAsync", () => {
let container = getContainer(container);
// actAsync returns a promise for all effects evaluation
actAsync(_ =>
// Convert ReactDOMRe.render to a promise
resolve(ReactDOMRe.render(effectfulComponent, container))
) |> then_(_ =>
// Call finish with assertion on final container
expect(
container
->DOM.findBySelectorAndTextContent("p", "Hello async Jest")
->Belt.Option.isSome) |> toBe(true) |> resolve
);
});

Slightly simpler and much safer.

Copy link
Author

Choose a reason for hiding this comment

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

That looks much better, unfortunately that does not seems to work with actAsync, here is the error message:

    TypeError: Cannot read property 'then' of undefined

      at Object.<anonymous> (node_modules/@glennsl/bs-jest/src/jest.bs.js:249:47)

Could it be caused by the actAsync binding implementation ( https://github.com/reasonml/reason-react/blob/5ae20e8a6245a584a8f452bf797a2ac26c09d873/src/ReactTestUtils.re#L22-L28 ) ?

Copy link
Author

Choose a reason for hiding this comment

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

For some reason, it seems like actAsync resulting promise can only be resolved once. e.g. actAsync(_ => () |> resolve) |> then_(_ => () |> resolve) is undefined.

Copy link
Owner

Choose a reason for hiding this comment

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

Huh, that's weird. They do seem to return a broken thenable. I can't imagine why they would do that without documenting it. Definitely seems like a bug, so perhaps it would be a good idea to open an issue on the react repo?

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

Successfully merging this pull request may close these issues.

2 participants