Migrate integration tests to some Makefile/shell based format

[ezyang]

Currently, the process for package tests in Cabal is something like:

1. Create a new Haskell file and write a lot of boilerplate to call a bunch of Cabal/ghc-pkg commands
2. Wire it up to the main test caller
3. Call 'cabal test' and wait for the test suite to build
4. Actually run the test suite

This really sucks, for a few reasons:

1. It takes a lot of boilerplate to write the actual package test, way more than an equivalent shell script plus expected output to diff against would
2. Under the most normal workflow, we have to rebuild the test suite every time we make a change (e.g. tweak a command). This makes for a pretty annoying compile-run cycles
3. It's opaque: it's much harder to reverse engineer what the source level commands are being run

The way GHC does these tests is that it just has a test runner which runs some shell scripts (really a Makefile) which runs a test, and the diffs it against some expected output. There's a bit of fuzz to make it all work out, but it is quite nice. Cabal should grow something similar; maybe that would encourage more tests! (Maybe there is a good out-of-the-box thing we can use here.)

[BardurArantsson]

+1, if you (or someone else) could make this happen it would be great.

I guess a feasible way to go here would to put this new system in place and not bother converting the existing test cases? (I may be sufficiently motivated to do so because I may need to add some PackageTest cases soonish.)

EDIT: Removed a bit of nonsense; I wasn't really familiar enough with the way cabal-install's PackageTests currently work.

[BardurArantsson]

Given that one of my "contributions" had to be reverted a few days ago (basically due to an error caused by a lack of automated tests), I think I'll take a stab at an initial implementation this weekend. @ezyang : Just to make sure we're not duplicating effort: are you actively working on this, or...?

[ezyang]

Nope, have at it!

[BardurArantsson]

So, I think I have a basic test runner set up working now:

Using cabal: /home/bardur/wd/cabal/cabal-install/dist/build/cabal/cabal
Using ghc-pkg: /usr/bin/ghc-pkg
Current directory is: /home/bardur/wd/cabal/cabal-install
Running tests in category 'cat1'...
  Running should_run tests...
    Found 3 test case(s)
    Running "see_me_fail.sh"...
    Running "see_me_succeed.sh"...
    Running "should_fail_because_of_output.sh"...
  Running should_fail tests...
    Found 1 test case(s)
    Running "does_not_fail.sh"...
Running tests in category 'cat2'...
  Running should_run tests...
    Found 0 test case(s)
  Running should_fail tests...
    Found 1 test case(s)
    Running "fails_as_it_should.sh"...

The file layout is like this:

tests/IntegrationTests
tests/IntegrationTests/cat1
tests/IntegrationTests/cat1/should_run
tests/IntegrationTests/cat1/should_run/see_me_fail.sh
tests/IntegrationTests/cat1/should_run/see_me_succeed.sh
tests/IntegrationTests/cat1/should_run/see_me_succeed.out
tests/IntegrationTests/cat1/should_run/should_fail_because_of_output.sh
tests/IntegrationTests/cat1/should_run/should_fail_because_of_output.out
tests/IntegrationTests/cat1/should_fail
tests/IntegrationTests/cat1/should_fail/does_not_fail.sh
tests/IntegrationTests/cat2
tests/IntegrationTests/cat2/should_run
tests/IntegrationTests/cat2/should_fail
tests/IntegrationTests/cat2/should_fail/fails_as_it_should.sh

(I'll probably rename to something other than IntegrationTests, but you get the idea.)

Each of the shell scripts can have an accompanying ".out" and ".err" file alongside it which will be compared against the stdout/stderr of running the script.

(At the moment I'm not actually running the shell scripts nor comparing stdout/stderr, but that should be pretty easy to do -- I'm mostly just working on the overall setup wrt. locations of test cases/data/etc. at the moment.)

A few of things to think about:

• Should scripts be run using "sh -e" as a wrapper? This would mean that any commands in the scripts which fail (and whose exit status isn't explicitly ignored using "|| true") will automatically cause the test case to fail... but then what about the "should_fail" tests?
• There needs to be some way of isolating tests and specifically having separate .cabal files for each test (if necessary), but it should probably also be possible to share .cabal files across multiple individual tests (shell scripts). I'm not sure how that should be done. Each test should perhaps also have its own separate working directory.
• What kind of state do the tests need access to? Should we provide the location of the built "cabal" executable via $PATH (and risk them using the system-wide one, or perhaps stripping PATH of everything else) or via a $CABAL environment variable? What other paths will test cases want to know? Perhaps this is one that can be left up to future expansion -- save the location of the "cabal-install" executable.

Comments/thoughts?

[phadej]

For should fail tests:

sh-3.2$ ! true
sh-3.2$ echo $?
1
sh-3.2$ ! false
sh-3.2$ echo $?
0

[ezyang]

You can always crib the relevant design decisions from GHC's test suite:

1. Make always stops on the first error
2. This is something GHC does quite poorly, which has caused us some pain. I think it would adequate if each Cabal calls has a dist directory suffixed with the testname; no need for a fresh working directory!
3. GHC does this with TEST_HC (which has the path to the GHC executable) and TEST_HC_OPTS (which is the options it should be called with.) I think this is pretty godo.

[BardurArantsson]

Here's the work so far:

https://github.com/BardurArantsson/cabal/tree/gh-2797

or as a single commit:

BardurArantsson@5e2a05b

The code uses the usual "tasty" infrastructure, so we get color, timing information and failed/success counts for free.
For now, I've marked outstanding issues in the code with "XXX". Here are the main remaining issues:

1. The current way of reading stdout/stderr is almost guaranteed to deadlock for non-trivial examples. This should be easily solvable by sparking off a couple of separate threads to perform the reading. (I wonder if it would be permissible to add async as a test dependency? I don't see any mention of it as a dependency in the .cabal files, but stack.yml mentions it, so presumably it's been there at some point.)
2. Right now I'm calling /bin/sh, but how would that work on Windows? (Presumably the current PackageTests test suite is cross-platform by virtue of invoking only "cabal".) Is it reasonable to require MSYS or similar to be able to run the test suite? I see that the current PackageTests/Exec/Check.hs runs "bash", so something must be going on here...
3. I'm providing a few environment variables so that "cabal" can be invoked properly (CABAL, CABAL_CONFIG_FILE, GHC_PKG, etc.), but I really want to make this simpler such that "cabal blah blah" is possible in the common case. I was thinking of maybe auto-generating a little wrapper shell script which defines a "cabal" shell function.
4. The should_fail tests could be a bit error-prone as-is. I'm thinking that perhaps we should require that either .out or an .err file be provided (or both) for all test cases, just to make sure that stupid things like misspelled commands don't end up causing a successful test (because of the "-e" flag to "sh").
5. Handling of the "dist" directory per @ezyang's comment: Yup, this is probably a good idea. Right now it's just set to "dist".

I still haven't rewritten any of the actual existing PackageTest tests, so it remains to be seen if I've forgotten anything crucial.

[BardurArantsson]

Oh, yes, here's an example of the output (sans color):

Current directory is: /home/bardur/wd/cabal/cabal-install
Integration Tests
  cat1
    should_run
      see_me_fail.sh (ignoring stdout+stderr):                         FAIL
        Unexpected exit status: 1
      see_me_succeed.sh (ignoring stderr):                             OK
      should_fail_because_of_output.sh (ignoring stderr):              FAIL
        <stdout> did not match file '/home/bardur/wd/cabal/cabal-install/tests/IntegrationTests/cat1/should_run/should_fail_because_of_output.out'. Was: "unexpected\n"
    should_fail
      does_not_fail.sh (ignoring stdout+stderr):                       FAIL
        Unexpected exit status: 0
  cat2
    should_fail
      fails_as_it_should.sh (ignoring stdout+stderr):                  OK
      succeeds_which_should_fail_the_test.sh (ignoring stdout+stderr): FAIL
        Unexpected exit status: 0

4 out of 6 tests failed (0.01s)

(I auto-add the "ignoring ..." bit just to make sure that it's possible to see at a glance if the discovery code is finding the .out and .err files as expected. I'm not sure if we can make it even more in-your-face or if we need to.)

[ezyang]

1. I don't see any problem with adding an async for the test suite
2. GHC's test suite works on Windows by assuming that Make is available, since you can't dev GHC without MSYS. Honestly, if someone is dev'ing on Windows I think it's OK to assume msys. Make sure you test I assume.
3. I don't like a wrapper script. It's an extra point of indirection, and if I want to run the commands manually I have to know to do the wrapper script. GHC's test suite, has fairly long command lines, but this makes it very explicit and easy to understand.

[BardurArantsson]

I've pushed an update which now reads the sub-process stdout/stderr asynchronously.

Yeah, I'll just go with "bash" for the shell, I think. Since it's only a requirement for development, I guess the MSYS requirement isn't too onerous -- and as I mentioned it seems to be a de-facto requirement for the existing test suite due to its invocation of "bash".

About the wrapper script: I can see your point, but I should think it would be a good thing to avoid a) excess bolierplate, but perhaps more importantly avoid b) accidentally calling the system-wide "cabal" executable by accident -- it's easy to forget that you have to use "special syntax" in test suites when reviewing code, etc. Hence I'd want to shadow the name "cabal" in test scripts anyway -- perhaps just to provide an appropriate error message about using $CABAL $CABAL_ARGS instead. What do you think about the latter approach?

[BardurArantsson]

Pushed a further little update:

• Uses withAsync now for automatic cleanup in case of exceptions. (Not really a big deal, but I guess it can't hurt.)
• Creates a working directory for each test case now and cleans it up after the test runs. The working directory is called ".work" relative to the test case directory. This is necessary because a) cabal is very stateful (fs-wise) and so we need a clean slate for each test case, and b) cabal tends to show paths in output, so we want relative paths for e.g. --config-file and CABAL_BUILDDIR.

Next up:

• Use "bash" instead of "sh". (Not sure if I can use the one in $PATH simply, but if not, I think I might just use /bin/bash.)
• Port a few tests over from PackageTests
• (Maybe:) Implement a wrapper script which "bans" the "cabal" command by defining a "cabal" function which displays an error if called. (See previous comment for reasoning.)

I think that should about do it.

[BardurArantsson]

Pushed a little update:

• Migrated the "cabal freeze" tests to integration-test. Net gain in LoC is ~90 lines. (Not that that should be the driving force, but it's a nice little bonus.)

Please have a look and see what you think!

This has uncovered a new potential source of problems, namely "unpredictable" file system state created by cabal-install and the need to do proper cleanup in a simple and reliable way.

In the case of "cabal freeze" it's cabal.config, and when porting these test cases I have resorted to deleting "cabal.config" in a "common.sh" script which is sourced by each of the test cases at the start. It would perhaps be a lot nicer if we just copied all the test data to a (temporary) work directory and removed the directory afterwards. (Assuming we didn't want to keep the work directory around for debugging the test case.)

[ezyang]

You mean "loss" :)

Can't you just set the --config-file to specify a separate config file? Would prefer not to be slinging files to random temporary directories, prefer it to be deterministic so we can look at the state if we're debugging.

[ttuegel]

Use "bash" instead of "sh". (Not sure if I can use the one in $PATH simply, but if not, I think I might just use /bin/bash.)

Please use either /bin/sh (required to exist by POSIX, but might not be bash) or /usr/bin/env bash (required to exist by LSB, but also present on all other Unices) as /bin/bash does not exist on all systems, even if bash is installed.

[ezyang]

I think it might just be better to use Make, in this case?