Lwt: [easy] Tests for the simpler modules of Lwt (multiple PRs)

[aantron]

This issue is about writing new test cases for some of the simpler helper modules of the Lwt core library:

• [x] Lwt_result (src/core/lwt_result.ml) – not tested 100% coverage by @jsthomas.
• [x] Lwt_condition (src/core/lwt_condition.ml) – not tested 100% coverage by @jsthomas.
• [x] Lwt_list (src/core/lwt_list.ml) – 22% 59% 100% coverage by @ryanslade.
• [x] Lwt_mvar (src/core/lwt_mvar.ml) – 33% 100% coverage by @jsthomas.
• [x] Lwt_pool (src/core/lwt_pool.ml) – not tested 100% coverage by @jsthomas.

Each of these is a separate small project.

Not included in this issue are:

• Lwt itself. This got thorough test cases recently in #339.
• Lwt_stream. This one needs tests improved, but the task is not an easy one. See #250 for a subset of the semantic subtleties of Lwt_stream.
• Lwt_mutex and Lwt_switch have pretty decent coverage (93% and 96%), though they could probably use a second look.
• Lwt_log_core needs testing, but, like Lwt_stream, this is not a very easy task.
• Lwt_sequence and Lwt_pqueue could use some more tests, and they should be easy to test. We consider them mostly as implementation details, however, so writing tests for them is not a high priority right now. You are still welcome to work on this, if you find these modules somehow attractive :) All improvements are welcome.

Note: high coverage is necessary but not sufficient for a good test suite. The above text is using it only as a proxy for true thoroughness.

Workflow

1. Pick one of the modules above. Leave a comment in this issue to claim it, so that others don't work on it at the same time. Say you picked Lwt_pool.

2. Clone the Lwt repo following the instructions in CONTRIBUTING.md.

3. Find or create tests/core/test_lwt_pool.ml. Edit or create the test suite in that file. It's okay to replace an existing test suite fully, if you want to be more organized about which test cases test what – but try to ensure that the new test suite tests everything the old suite did.

   For an example of a test suite, refer to the code of the modern test suite for module Lwt in tests/core/test_lwt.ml. You don't have to structure the code like that (repeated concatenation of test lists). That was only done for sectioning, since the test suite for module Lwt is huge (300+ cases). A simpler example is the older tests/core/test_lwt_mutex.ml. This older test suite has slightly messier cases, but the overall structure is much simpler, because it is not sectioned.

   Lwt uses its own testing framework (not exactly sure why). You can probably learn everything you need from the above examples. However, just in case, its interface is found in tests/test.mli.

4. If you created a new file, list its new module in tests/core/main.ml.

5. Run the tests with coverage enabled. For this, do:

   opam pin add --dev-repo bisect_ppx
   make coverage
   

   and view the generated file _coverage/index.html. It will look something like this. Navigate to the file you are testing, whose report might look like this.

   Coverage analysis will suggest more tests cases to write, but, ideally, a thorough test suite will include test cases beyond the minimum necessary to achieve high coverage. Think of the possible combinations of states and/or inputs, and try to test as many of them as seems reasonable. Don't go crazy here – if testing something feels excessive and too much of a chore, we would probably agree with you, so don't test it :) If you want, you can leave a note, or bring it up in this issue, or in your posted PR, and we will probably confirm that it's not worth the trouble to test :)

   A good rule of thumb is to put everything into some known state in the easiest way possible at the beginning of a test case, and then test one kind of transition.

   It is recommended to write as few helpers as possible outside your test cases. Test cases serve as a form of documentation, so it's good if they are crisp and self-contained. However, helpers are sometimes unavoidable.

6. Once finished, send a PR. Thanks!

   If you find any bugs while writing tests, send additional PRs. Additional thanks! :)

Background

About testing

Many people already know that testing is a good thing, so at the risk of being repetitive: thorough testing increases confidence in Lwt, and makes it easier to refactor code, freeing up contributor time and reducing contributor anxiety. Helping contributors helps users in the long term. Plus, bugs are often found while writing tests.

Lwt is already headed in the direction of thorough testing. We have added some tests over the last few months; in particular #339 added a big new test suite for lwt.ml, and #319 added coverage analysis for OCaml code. We want to continue along this direction, but most of the rest of the test suite is still very much inadequate.

About the core library

The "core library" of Lwt is all the pure-OCaml Lwt code, that does not assume a Unix system – it's the half of Lwt that's portable between native code and JavaScript. Basically, it's module Lwt itself, and some pure-OCaml helper modules, Lwt_list, Lwt_result, Lwt_mutex, etc. – a bunch of synchronization primitives and data structure helpers. If you want to see the full list, glance at the manual. The core library modules appear under heading "Core library" in the table of contents on the left.

The core library has two big modules, Lwt and Lwt_stream. The rest are relatively simple.

[jsthomas]

I'd like to work on this issue, specifically testing Lwt_result. However, I'm hitting an error when I try to assess coverage on my copy of the working-coverage branch (i.e. my copy with no new changes). When I run make coverage, I get:

./setup-dev.exe -build 
W: Cannot find source file matching module 'Lwt_unix' in library lwt-unix.
W: Use InterfacePatterns or ImplementationPatterns to define this file with feature "source_patterns".
W: No exported module defined for library lwt-simple-top
W: No exported module defined for library lwt-syntax-options
W: Cannot find source file matching module 'Lwt_unix' in library lwt-unix.
W: Use InterfacePatterns or ImplementationPatterns to define this file with feature "source_patterns".
W: No exported module defined for library lwt-simple-top
W: No exported module defined for library lwt-syntax-options
W: Cannot find source file matching module 'Lwt_unix' in library lwt-unix.
W: Use InterfacePatterns or ImplementationPatterns to define this file with feature "source_patterns".
W: Cannot find source file matching module 'Lwt_unix' in library lwt-unix.
W: Use InterfacePatterns or ImplementationPatterns to define this file with feature "source_patterns".
+ ocamlfind ocamlopt -package unix -package ocamlbuild -linkpkg -package cppo_ocamlbuild myocamlbuild.ml /usr/lib/ocaml/ocamlbuild/ocamlbuild.cmx -o myocamlbuild
File "myocamlbuild.ml", line 1:
Error: /usr/lib/ocaml/ocamlbuild/ocamlbuild.cmx is not a compilation unit description.
Command exited with code 2.
Compilation unsuccessful after building 1 target (0 cached) in 00:00:00.
E: Failure("Command ''/usr/bin/ocamlbuild' src/core/lwt.cma src/core/lwt.cmxa src/core/lwt.a src/core/lwt.cmxs src/logger/lwt-log.cma src/logger/lwt-log.cmxa src/logger/lwt-log.a src/logger/lwt-log.cmxs src/unix/liblwt-unix_stubs.a src/unix/dlllwt-unix_stubs.so src/unix/lwt-unix.cma src/unix/lwt-unix.cmxa src/unix/lwt-unix.a src/unix/lwt-unix.cmxs src/simple_top/lwt-simple-top.cma src/simple_top/lwt-simple-top.cmxa src/simple_top/lwt-simple-top.a src/simple_top/lwt-simple-top.cmxs src/preemptive/lwt-preemptive.cma src/preemptive/lwt-preemptive.cmxa src/preemptive/lwt-preemptive.a src/preemptive/lwt-preemptive.cmxs tests/test.cma tests/test.cmxa tests/test.a tests/test.cmxs tests/core/main.native tests/unix/main.native tests/preemptive/main.native -use-ocamlfind -plugin-tags  'package(cppo_ocamlbuild)' -tag debug -tag tests' terminated with error code 10")
Makefile:33: recipe for target 'build' failed
make: *** [build] Error 1

If it helps, I'm running with bisect_ppx 1.2.0, oasis 0.4.10, and ppx_tools_versioned 5.0.1. These were the dependencies I found I needed to install to get through the first two steps preceding make coverage.

[aantron]

As a first guess, can you try running make clean distclean, and then starting the build over?

[aantron]

If you/we don't solve this in about 10 minutes, then it'll be the kick for getting some kind of hacky Bisect support merged into master (e.g. #419). Don't spend too much time on this; I can take of a hacky Bisect setup tomorrow.

[jsthomas]

Running make clean distclean did the trick, thanks. I'll start in on writing some tests for Lwt_result now.

[aantron]

@jsthomas Now that #438 is in, we have coverage working for the core library in master. I'll still keep working-coverage around for a while, so you don't have to transfer your work over.

[jsthomas]

I'm working on tests for Lwt_mvar now (based off of the master branch, not working-coverage).

[aantron]

@jsthomas beware https://github.com/ocsigen/lwt/issues/443. cc @hcarty

[hcarty]

Getting what's in Lwt_mvar now covered is 👍 - I/we can add tests for new functions as they arrive. Please don't let my potential future work slow you down!

[jsthomas]

I'm working on tests for Lwt_condition now.

[jsthomas]

I'm working on tests for Lwt_pool now.

[aantron]

You're destroying this issue, I need to find new ones :p

[jsthomas]

I have a question regarding how validate is supposed to work in Lwt_pool.create: Is validate supposed to be called on every call to use? Based on what I read here, I thought so, but I have a test that doesn't yield the results I expected. Specifically, the passes, but I would've thought I should have Lwt.state u2 = Lwt.Fail Dummy_error.

[aantron]

The docs and the code both seem to imply that validate is called before every return of an element from use, except for new members of the pool.

However, in case there are no elements available to use immediately and the pool limit is already reached, it looks like acquire (used internally by use) has the caller wait for an element to become available, and I see nothing to indicate that the element, when it becomes available, is ever passed through validate. This seems like a bug, since effectively it looks like elements are only passed through validate if the pool doesn't have to wait to acquire them. I encourage you to stare at the code a bit longer and confirm it. Also cc @hcarty and anyone else interested in Lwt_pool.

A separate PR, whether before or after the tests, is welcome for fixing this :) It's a good find. Before is probably easier, as we can then get correct tests for correct behavior merged in one PR, instead of having to adjust them after the fix.

[aantron]

Alternatively, you are welcome to open an issue instead of a PR, or prod me and I'll do it :)

[jsthomas]

Thanks for confirming, I will work on a fix for this in a seperate PR.

[aantron]

So, thanks to @ryanslade and @jsthomas, the core now has a really good test suite.

The status of testing of the core now is that there are a few really specific testing issues open:

• Test order in which cancelation and resolution happen in pick, npick (#463).
• Test interaction of Lwt.finalize and Lwt.cancel (#452).
• Tests for Lwt_sequence (#406) – this is an internal module.
• Involved tests for Lwt_list (#395) – more explicit testing of concurrency.
• Lwt_stream is not well-tested. I'll write an issue about it.

Most of these are responses to specific questions or issues we got in the last months, where there turned out to be a testing gap.

We also need to get started writing test for the Unix library later (Lwt_unix, Lwt_io, etc).