Make alcotest-lwt compatible with jsoo#348
Conversation
|
gentle ping |
|
Thanks @hhugo. I'll review this (& go through backlog elsewhere) this weekend 👍 |
TheLortex
left a comment
TheLortex
left a comment
There was a problem hiding this comment.
Thanks for this PR.
The changes are making sense to me. However I'm afraid of the breaking changed introduced by the removal of the lwt.unix dependency. I'll try to investigate and see which packages are involved.
I have added a comment about alcotest_test_helper, we should explain somewhere why it is used instead of Lwt_main.run.
Can you rebase on top of main and add a changelog entry ?
| fmt | ||
| alcotest.engine | ||
| alcotest | ||
| (re_export lwt.unix) ;; Unused dependency added for convenience to consumers |
There was a problem hiding this comment.
This looks like a breaking change.. I don't know how many packages depend on lwt.unix being re-exported though.
| let test_lwt switch () = | ||
| Lwt_switch.add_hook (Some switch) free; | ||
| Lwt.async (fun () -> failwith "All is broken"); | ||
| Lwt_unix.sleep 10. |
There was a problem hiding this comment.
What was the reason for this 10 seconds sleep ?
| @@ -0,0 +1,7 @@ | |||
| let rec wakeup_until_resolved p = | |||
There was a problem hiding this comment.
It should be explained somewhere that to make the tests work with js we use a custom lwt event loop.
Either in CONTRIBUTING.md or in alcotest_test_helper.ml.
|
@hhugo what's the status of this? |
|
I don't have time to spend on this and no longer have a need for this at the moment. Feel free to close or takeover the PR |
4 participants
No description provided.