Issue 395: Add tests for stack safety in Lwt_list. #538
Conversation
There was a problem hiding this comment.
@jsthomas, except for the nits, these are great, and indeed these functions are all tail-recursive now.
I was going to merge this by flipping the order of the commits, to keep a history in which tests are always passing (squashing works too). But after I found the nits, I figured I better offer you to fix them, but if you want I can fix them as well.
It took 10 million list cells to get some of the functions to hit the stack limit on my machine, instead of 1 million. However, that also makes the tests run awfully slow, so in future work, we should probably separate the tail-recursion test suite out, and concatenate it onto the test list only if some command line argument is given to the tester (or an environment variable is set). That way, we can run all the tests in Travis with huge lists, but skip them for ordinary day-to-day development.
If you want to try that in this PR, go ahead, otherwise I'll open an issue for it.
Related: #347.
test/core/test_lwt_list.ml
Outdated
| test_big_list m | ||
| end; | ||
|
|
||
| test "filter_map_s serlialism" begin fun () -> |
There was a problem hiding this comment.
I think the title is wrong here :)
| test_big_list m | ||
| end; | ||
|
|
||
| test "filter_map_p big list" begin fun () -> |
There was a problem hiding this comment.
I think this test and the next one are duplicates of two tests above.
There was a problem hiding this comment.
Good catch, thanks.
|
If you do want to make the tail-recursion suite conditional though, the tester's Lines 40 to 41 in 4f0dabc |
test/core/test_lwt_list.ml
Outdated
| m f (make_list 10_000_000) >>= (fun _ -> Lwt.return_true) | ||
|
|
||
| let suite_intensive = suite "lwt_list big lists" | ||
| ~only_if:(fun () -> Sys.getenv_opt "LWT_STRESS_TEST" <> None) [ |
There was a problem hiding this comment.
getenv_opt requires 4.05, so this won't compile on most versions of OCaml Lwt still supports.
|
Thanks! I modified |
This change proposes new tests to verify that functions in
Lwt_listare stack safe, which addresses the third and final part of #395.The first commit adds new tests, applying each function in
Lwt_listto a list of one million elements. (I also updated the labels on tests in #535, to make the terminology a little more consistent; now we use the term "serialization" everywhere). These tests do not pass for several of theLwt_listfunctions.The second commit updates
Lwt_listto make the tests pass. I've tried to introduce the[@ocaml.tailcall]where appropriate to make our use of tail recursion more explicit.