Lwt: Fix the PPX expect tests

[aantron]

See comments starting from https://github.com/ocsigen/lwt/pull/474#issuecomment-336633064:

The tests can't find lwt.ppx when lwt isn't installed through opam (which is typical for me during development). In other words, they have been running against an installed Lwt, not against the Lwt in the working tree.

[raphael-proust]

I'll have a look into this. Might take me some time because I don't know much about jbuilder or opam.

[aantron]

@raphael-proust Great, thanks! I'm sure you saw the branch linked from #474. Despite my earlier comment, I think the easiest way to get this done is to get the tests to respect OCAMLPATH somehow, or an equivalent approach, not to make tons of Jbuilder executables. You may want to browse the Findlib code for this, it can be found here on GitLab: https://gitlab.camlcity.org/gerd/lib-findlib. It's actually pretty legible as far as OCaml code goes.

Under normal circumstances, OCAMLPATH can be set to cause Findlib to look for packages in certain directories. However, #require (provided by Findlib) does not seem to read OCAMLPATH, so we can't use this trick to point Findlib into Lwt's _build directory.

I may, of course, still be wrong, and Jbuilder might be easier :p

[raphael-proust]

I cannot reproduce this failure. I uninstalled lwt (unpinned it really), updated everything, and the ppx part is fine:

File "test/ppx/main.ml", line 162, characters 43-45:
Warning 22: The operator >> is deprecated
File "test/ppx/main.ml", line 161, characters 43-45:
Warning 22: The operator >> is deprecated
    ocamlopt test/ppx/main.{cmx,o}
    […]
    ocamlopt test/ppx/main.exe

Can you confirm it is still happening? It might have been fixed by some other commit.

[aantron]

@raphael-proust, I temporarily disabled the PPX expect tests in a commit, https://github.com/ocsigen/lwt/commit/3be9cc4f8bce259b3323ea2263a5bd15032d8815. You'll have to uncomment that jbuild file as part of your branch :)

The tests are in a separate directory test/ppx_expect BTW. I also just realized that directory name can be confusing with opam package ppx_expect, but it's not a big deal.

[raphael-proust]

Progress notes (mostly for myself so I can resume investigation tomorrow).

The OCAMLPATH variable is unavailable even before the #require directive is processed. It is not in the env of commands started by jbuild.

This is tested like so:

(alias
 ((name runtest)
  (package lwt)
  (deps ((files_recursively_in cases)))
  (action (system "echo OCAMLPATH=$OCAMLPATH"))))

It is easy to set a custom environment in jbuilder

  (action
    (setenv OCAMLPATH ../../_build/install/default/lib/lwt/
    (run ${exe:main.exe})))))

But it still fails. I'll have a look at whether this is the wrong OCAMLPATH value, or if it's Findlib's #require.

[aantron]

Yes, OCAMLPATH is typically unset to begin with in your shell.

Purely as FYI, I typically set OCAMLPATH in the shell, externally to Jbuilder, e.g.: https://github.com/aantron/bisect_ppx/blob/86ca173aada2a080d884f2bca2204f40db88bd8b/test/src/test_helpers.ml#L186.

However, using Jbuilder for this is probably more portable, so a good idea. I may have used it, but the code above was written before that project was ported to Jbuilder.

[aantron]

Hey @raphael-proust, I'm going to take over this so we can put the ;%lwt test in it (#307), hope that's okay.

[aantron]

Well, I restored the tests in a branch, but committing is best done after the PPX is fully factored out (#515, #338). Until then, the interaction of the expect tests with the Lwt build system is just too messy.

[raphael-proust]

@aantron yeah, sorry I went inactive

[aantron]

It's no problem at all :)

[smithjessk]

According to Travis, a1fc678 didn't fix the issue for make coverage. But, I fixed this locally by building the lwt package using BISECT_ENABLE=yes make build inside the coverage task:

.PHONY: coverage
coverage: clean check-config
    BISECT_ENABLE=yes make build
    BISECT_ENABLE=yes jbuilder runtest --dev -j 1 --no-buffer
    bisect-ppx-report \
        -I _build/default/ -html _coverage/ \
        -text - -summary-only \
        $(BISECT_FILES_PATTERN)
    @echo See _coverage/index.html

Is this an acceptable fix? Is there anything else I should be worried out? I'm still pretty new to jbuilder, etc. If this works, I'll put it in a PR to the ppx-expect branch. Thanks!

[aantron]

Absolutely, thanks for taking a look! If the tests are working, and the coverage report looks correct before and after, then it is an acceptable fix indeed :)

The most important thing to watch out for is that lwt and lwt_ppx are both uninstalled in your opam switch when you are testing the expect tests. This is to ensure that the tests are not picking up the installed versions of those packages, and are able to work with the development versions built by Jbuilder in ./_build.

[smithjessk]

Yep- uninstalled both of them and the coverage tests completed. Opened a PR, too. Thanks, @aantron!