Issue #385: Increase coverage for Lwt_mvar #448
Conversation
test/core/test_lwt_mvar.ml
Outdated
| (fun () -> | ||
| let x = Lwt_mvar.create 0 in | ||
| let y = Lwt_mvar.put x 1 >>= (fun _ -> return 1) in | ||
| return (y != Lwt.return 1) |
There was a problem hiding this comment.
Is this intentionally using != rather than <>?
There was a problem hiding this comment.
No, that was a typo. Thanks for catching that.
test/core/test_lwt_mvar.ml
Outdated
| let _ = Lwt_mvar.take y >>= (fun z -> Lwt_mvar.put x z); in | ||
| () in | ||
| let _ = Lwt.on_cancel r1 after_cancel in | ||
| Lwt.cancel r1; |
There was a problem hiding this comment.
Changing this test should allow you to get that remaining line fully covered. Indeed, I think in this form, this test doesn't add anything beyond the tests above.
Since t is still pending at this point, when you cancel r1, it cancels t, and the bind that creates r1 falls through bind's exception-forwarding case. So the function that puts 2 to y is simply never called, the put is never even started or tried, and this is only testing how bind responds to cancel.
To trigger that line, what this test should do is get rid of t, and define r1 as just that put y 2. It is put y 2 that has an on_cancel handler added to it in Lwt_mvar (and we currently never call it).
This will also be a pending promise, because y already has 1 at that moment. You'll want to cancel r1, take from y, check that it's 1, take again, and check that the taker is blocked the second time.
I don't think x is necessary.
The reader test below has a similar issue.
There was a problem hiding this comment.
Thanks, that worked and is a lot simpler than the original.
test/core/test_lwt_mvar.ml
Outdated
| (fun () -> | ||
| let x = Lwt_mvar.create 0 in | ||
| let y = Lwt_mvar.put x 1 >>= (fun _ -> return 1) in | ||
| return (y <> Lwt.return 1) |
There was a problem hiding this comment.
This is optional (the test is correct regardless), but I believe you could do:
(* ... *)
let y = Lwt_mvar.put x 1 in
Lwt.return (Lwt.state y = Lwt.Sleep)This makes the test slightly clearer, and it would be directly testing that y is pending. Actually, I would recommend always checking the state of promises with Lwt.state rather than comparing to another promise, as comparing with another promise will fail if the internal definition of promises ever changes to include, say, unique ids for tracing.
Sorry about the name Lwt.Sleep for "pending," that's a historical holdover :)
test/core/test_lwt_mvar.ml
Outdated
| *) | ||
|
|
||
| open Test | ||
| open Lwt |
There was a problem hiding this comment.
This is optional, but I recommend removing this (sorry if you saw it used in another test suite). Your code already nicely uses Lwt. in most places, removing it would, I think, just make the returns at the end of each test more explicit.
test/core/test_lwt_mvar.ml
Outdated
| let x = Lwt_mvar.create 0 in | ||
| let _ = Lwt_mvar.take x in | ||
| let z = Lwt_mvar.put x 1 >>= (fun _ -> return 2) in | ||
| return (z = Lwt.return 2) |
There was a problem hiding this comment.
The above optional suggestion about Lwt.state would save a bind here as well, and the need for 2 :) Another way is to compare with Lwt.return (), but... Lwt.state :)
test/core/test_lwt_mvar.ml
Outdated
| let r1 = Lwt_mvar.put y 2 in | ||
| Lwt.cancel r1; | ||
| return ((Lwt_mvar.take y = Lwt.return 1) | ||
| && (Lwt_mvar.take y >>= (fun _ -> Lwt.return 2) <> Lwt.return 2))) |
There was a problem hiding this comment.
Lwt.state should make this final condition substantially clearer.
|
(Also want to link this with #385, GitHub only creates a link if you mention the issue number in a comment, not in the PR title). |
|
Thanks for the feedback; |
|
Thanks, this is excellent! |
This change adds some unit tests for mailbox variables (
Lwt_mvar), bringing coverage up to 100%.