Switch testing from ad-hoc scripts to OUnit

[aantron]

The code is largely complete, but I intend to amend the commit before it's ready for merging. I have a bunch of questions/notes first, though.

I intend to:

• Add some basic docs on how the testing framework is designed. It runs each test case in a temporary directory tests/_scratch, but the best place to explain that is in the code.
• Look through .gitignore for obsolete entries.
• Adjust the new tests/Makefile to have features like the old one, and comment usage.
• Add some warnings to the test runner in case optional packages like ppx_blob aren't installed. We can make these fatal in Travis to make sure we are testing everything.

Before merging:

• Are we assuming ocamlfind now? Some of the older code relies on ./configure and doesn't assume ocamlfind, but the newer code makes the opposite choice. I went with the former.
• I want to confirm that we are assuming 4.02. I think ppx_tools requires it, but ppx was supported experimentally in 4.00 (or was it?). I'm in favor of assuming 4.02, it makes it possible to assume |> and @@ :) Perhaps we should add a constraint to opam for clarity.
• Should the PR close issue Change the testing infrastructure #2? It doesn't literally resolve the issue mentioned in the original description. It just translates the Makefiles to OUnit for now. Hopefully, that will make it easier to adjust the tests consistently in the future.
• What license headers should I add to the new files?
• The ppx-deriving directory seems like it should be renamed, because it contains tests with various ppx rewriters, not just ppx_deriving.
• The other ppx-* directories are named that way from the time when ppx was an alternative in this code base to camlp4. The prefix is confusing with ppx-deriving. Let me know if you object to dropping the prefix from these.

Other notes:

• ppx_blob tests are failing due to Warnings about compiler-libs when using ppx_blob johnwhitington/ppx_blob#1. I skipped them.
• The ppx_deriving test has been failing without us noticing, because the testing framework ignored the exit code. For example: https://travis-ci.org/rleonid/bisect_ppx/jobs/102869628#L507. The translated version is still failing now, for the same reason, but I marked it skipped, and we can fix it in a separate change.
• There was some dead code in directory ppx-deriving for testing ppx_env and such. I deleted this. We can recover and adapt it later from the history.
• I will apply bisect_ppx to itself in a future PR. I want to keep this one focused :)
• combine-expr tests 1 and 2 have the same output. Might want to avoid that :)

The output of make tests (after make all) now looks something like this:

test -f `pwd`/tests/Makefile && (cd `pwd`/tests && /Applications/Xcode.app/Contents/Developer/usr/bin/make --no-print-directory all && cd ..) || true
ocamlbuild -use-ocamlfind -no-links -cflags -w,+A-4-48 test_runner.native -- -runner sequential
Finished, 38 targets (36 cached) in 00:00:00.
......................................SSS.
Ran: 42 tests in: 4.93 seconds.
OK: Cases: 42 Skip: 3

In case of failure, you get a non-zero exit code and output like this:

test -f `pwd`/tests/Makefile && cd `pwd`/tests && /Applications/Xcode.app/Contents/Developer/usr/bin/make --no-print-directory all
ocamlbuild -use-ocamlfind -no-links -cflags -w,+A-4-48 test_runner.native -- -runner sequential
Finished, 38 targets (38 cached) in 00:00:00.
....F.................................SSS.
==============================================================================
Error: bisect_ppx:0:report:4:html.

File "/Users/antron/Coding/Current/bisect_ppx/tests/_build/oUnit-bisect_ppx-antron#00.log", line 32, characters 1-1:
Error: bisect_ppx:0:report:4:html (in the log).

Raised at file "src/oUnitAssert.ml", line 45, characters 8-27
Called from file "src/oUnit.ml", line 211, characters 16-25
Called from file "src/oUnitTest.ml", line 129, characters 16-22
Re-raised at file "src/oUnitTest.ml", line 134, characters 12-13
Called from file "src/oUnit.ml", line 86, characters 13-17
Called from file "src/oUnit.ml", line 86, characters 13-17
Called from file "src/oUnit.ml", line 97, characters 13-17
Called from file "src/oUnitRunner.ml", line 46, characters 13-26

Difference against 'report/reference.html':

3c3
<     <title>Bisect report foobar</title>

---
>     <title>Bisect report</title>

------------------------------------------------------------------------------
Ran: 42 tests in: 5.00 seconds.
FAILED: Cases: 42 Tried: 42 Errors: 0 Failures: 1 Skip:  3 Todo: 0 Timeouts: 0.
make[1]: *** [all] Error 1
make: *** [tests] Error 2

[aantron]

The OS X build failed, but it looks like it has been failing since https://travis-ci.org/rleonid/bisect_ppx/builds/78852923 (4 months ago), just not noticed by Travis due to ignored exit status. I'm guessing you will have more insight into the cause than I.

By the way, I develop on OS X, and testing works fine locally.

[rleonid]

Are we assuming ocamlfind now? Some of the older code relies on ./configure and doesn't assume ocamlfind, but the newer code makes the opposite choice. I went with the former.

I don't mind assuming ocamlfind and not relying on a ./configure. I think that it is just more surface area (that could go wrong). My original intent was to not diverge too much from the original bisect code; but software and the constraints that seem reasonable change, so if it is hindering your progress that's fine.

I want to confirm that we are assuming 4.02. I think ppx_tools requires it, but ppx was supported experimentally in 4.00 (or was it?). I'm in favor of assuming 4.02, it makes it possible to assume |> and @@ :) Perhaps we should add a constraint to opam for clarity.

Those are good reasons, 4.02 it is. For the record, I think using ppx_metaquot for the actual driver (instrumentPpx) could greatly simplify the logic. But that is definitely beyond the scope of this PR.

Should the PR close issue #2? It doesn't literally resolve the issue mentioned in the original description. It just translates the Makefiles to OUnit for now. Hopefully, that will make it easier to adjust the tests consistently in the future.

I'd like to have a one-to-one mapping from Issue's to PR's, with the caveat that you can always add more issues to keep track of things that need to get done. So if this changes the infrastructure and there is another issue about checking for space, or another for property driven tests, that would be fine.

What license headers should I add to the new files?

I don't know. I tend to favor the loosest open source licenses possible as those reflect the amount of time I'm willing to worry about those issues. If you're not opposed, I would keep it GPL, out of respect to @xclerc original wishes (with your name on it).

The ppx-deriving directory seems like it should be renamed, because it contains tests with various ppx rewriters, not just ppx_deriving.

True.

The other ppx-* directories are named that way from the time when ppx was an alternative in this code base to camlp4. The prefix is confusing with ppx-deriving. Let me know if you object to dropping the prefix from these.

No objection, redundant usage of ppx is redundant.

[aantron]

Thanks for the replies. Don't spend time reading the existing diff please, because I am about to push a new one. Just have to spend some time separating the rename changes into a separate commit...

[rleonid]

ppx_blob tests are failing due to johnwhitington/ppx_blob#1. I skipped them.

Fine.

The ppx_deriving test has been failing without us noticing, because the testing framework ignored the exit code. For example: https://travis-ci.org/rleonid/bisect_ppx/jobs/102869628#L507. The translated version is still failing now, for the same reason, but I marked it skipped, and we can fix it in a separate change.

That's fine. I noticed that it was failing when you started submitting your patches. It is failing because of this bug in ocamlc 's -dsource output. I would like to keep the test though. One of my original stumbling blocks was making sure that this rewriter worked well with other ppx rewriters. That was the original intent for the ppx_deriving and ppx_blob series of tests.

There was some dead code in directory ppx-deriving for testing ppx_env and such. I deleted this. We can recover and adapt it later from the history.

The original intent was to make sure that both order of rewriters worked correctly: ppx_deriving -> bisect_ppx and bisect_ppx -> ppx_deriving. At the time I convinced myself that there were use cases for both. I've added this as another issue #35.

I will apply bisect_ppx to itself in a future PR. I want to keep this one focused :)

great. #36

combine-expr tests 1 and 2 have the same output. Might want to avoid that :)

great.

[rleonid]

Cool. Let me know when you're ready for a review. It looks great so far!

[aantron]

Ok, should be ready for review now. There is still the Travis OS X issue, but I don't think it's caused by these commits.

• If you want to get an overview, start with tests/README.md. Then, you may want to skim the internal interface in tests/test_helpers.mli.
• I changed the Travis script to install ppx_deriving and fail if optional dependencies for testing are not installed. Let's see what happens.
• I didn't add license headers to some of the really short files, because that seems ridiculous. Let me know if you prefer me to add them.

I don't know if you want to look into the OS X Travis build issue before merging this. I was going to take a look, but I was hoping you could offer some insight as to what may have caused it to appear back when it did.

[aantron]

Dang it, I can't seem to get xmllint installed correctly in Travis. Do you have access to an Ubuntu system? I tried packages libxml2 and libxml2-utils, but apparently which xmllint reports it is still missing. Is it not in PATH by any chance?

[aantron]

Scratch that, was using a BSD-specific option in the which invocation.