paurkedal / caqti-study

MIT License
8 stars 3 forks source link

Parallel tests are broken #9

Open benjamin-thomas opened 1 year ago

benjamin-thomas commented 1 year ago

I get random test failures, which I think are due to separate project's trying to setup the database at the same time.

For instance :

Quick-fix solution: build and/or test with one worker.


PGHOST=localhost PGDATABASE=caqti_study PGPORT=5433 dune build @all @runtest -j1

Or:

PGHOST=localhost PGDATABASE=caqti_study PGPORT=5433 dune build
PGHOST=localhost PGDATABASE=caqti_study PGPORT=5433 dune runtest -j1

It'd be nice to find a better solution. Maybe something like:

paurkedal commented 1 year ago

Dune has a (locks VIRTUAL_PATH) attribute which can be used to avoid conflicting concurrent runs (cf the Caqti testsuite). But I'll think some more about it and comment, since I'm having similar issues with test suites depending on DBs (and not just in Caqti).

Another motivation I have for rethinking this is that if someone (like myself) already has set up access to a personal database, then we want to avoid accidentally creating tables in its public namespace when the PG* variables are unset.

benjamin-thomas commented 1 year ago

Thanks for the tip!

To avoid accidentally creating or deleting tables, I can require mandatory prefixed env vars, something like:

Would that work for you?

Env vars are pretty flexible IMO, and make it so we never have to worry about committing sensitive data by accident.

We can simulate having something close to a config file for free using a tool such as envdir, from the daemontools package.

$ echo 1 >/tmp/env/CAQTI_HELLO
$ echo 2 >/tmp/env/CAQTI_WORLD
$ envdir /tmp/env/ env | grep CAQTI
CAQTI_WORLD=2
CAQTI_HELLO=1

# Or if that wasn't clear
$ envdir /tmp/env/ bash -c 'echo $CAQTI_HELLO'
1

So one could run the test suite like such:

envdir ~/env/caqti-study/ dune runtest
paurkedal commented 1 year ago

I think adding a prefix to the variables is good, since it allows people to set the variables globally if they wish, without affecting other applications. However, we would also need to drop the postgres:// fall-back, since that may point to the wrong database. But we could consider making the fall-back postgres://localhost:5433/local_db for convenience. It's already the default in the makefile, and I think it's safe enough, since the port is non-standard and the database name is unique enough (we could also call it caqtistudy or caqti_study).

(I didn't know about envdir, but an alternative is to put the environment variables in a script which executes the rest of the command-line at the bottom.)

benjamin-thomas commented 1 year ago

However, we would also need to drop the postgres:// fall-back, since that may point to the wrong database

Sounds good to me. I'll make this a failing condition.

an alternative is to put the environment variables in a script which executes the rest of the command-line at the bottom

Yes that's another way to do it. I'll write a little wrapper script to make things easy to manage.