Migrate io-tests from bash to pytest

[monacoremo]

As discussed in #1676 (comment), the io-tests are pushing bash to its limits and migrating to a python setup might be much more maintainable

Tests to migrate (Edit: updated from master)

• ok 1 - dump of config file configs/aliases.config is valid
• ok 2 - dump of config file configs/app-settings.config is valid
• ok 3 - dump of config file configs/base64-secret-from-file.config is valid
• ok 4 - dump of config file configs/boolean-numeric.config is valid
• ok 5 - dump of config file configs/boolean-string.config is valid [0/1614]
• ok 6 - dump of config file configs/dburi-from-file.config is valid
• ok 7 - dump of config file configs/defaults.config is valid
• ok 8 - dump of config file configs/no-defaults.config is valid
• ok 9 - dump of config file configs/role-claim-key.config is valid
• ok 10 - dump of config file configs/secret-from-file.config is valid
• ok 11 - dump of config file configs/sigusr2-settings.config is valid
• ok 12 - dump of config file configs/simple.config is valid
• ok 13 - dump of config file configs/unix-socket.config is valid
• ok 14 - dump of config file configs/aliases.config does match expectation
• ok 15 - dump of config file configs/boolean-numeric.config does match expectation
• ok 16 - dump of config file configs/boolean-string.config does match expectation
• ok 17 - dump of config file configs/defaults.config does match expectation
• ok 18 - dump of config file configs/no-defaults.config does match expectation
• ok 19 - Succesfully connected through unix socket
• ok 20 - authentication with simple (no EOL) secret read from a file
• ok 21 - authentication with simple secret read from a file
• ok 22 - authentication with ASCII (no EOL) secret read from a file
• ok 23 - authentication with ASCII secret read from a file
• ok 24 - authentication with UTF-8 (no EOL) secret read from a file
• ok 25 - authentication with UTF-8 secret read from a file
• ok 26 - authentication with binary secret read from a file
• ok 27 - authentication with binary (+EOL) secret read from a file
• ok 28 - authentication with Base64 (simple) secret read from a file
• ok 29 - authentication with Base64 (ASCII) secret read from a file
• ok 30 - authentication with Base64 (UTF-8) secret read from a file
• ok 31 - authentication with Base64 (binary) secret read from a file
• ok 32 - connection with (no EOL) dburi read from stdin / a file
• ok 33 - connection with (EOL) dburi read from stdin / a file
• ok 34 - request with ".postgrest.a_role" role-claim-key for {"postgrest":{"a_role":"postgrest_test_author"}} jwt: 200
• ok 35 - request with ".customObject.manyRoles[1]" role-claim-key for {"customObject":{"manyRoles": ["other", postgrest_test_author"]}} jwt: 200
• ok 36 - request with "."https://www.example.com/roles"[0].value" role-claim-key for {"https://www.example.com/roles":[{"value":"postgrest_test_author"}]} jwt: 200
• ok 37 - request with ".myDomain[3]" role-claim-key for {"myDomain":["other","postgrest_test_author"]} jwt: 401
• ok 38 - request with ".myRole" role-claim-key for {"role":"postgrest_test_author"} jwt: 401
• ok 39 - invalid jspath "role.other": rejected
• ok 40 - invalid jspath ".role##": rejected
• ok 41 - invalid jspath ".my_role;;domain": rejected
• ok 42 - invalid jspath ".#$%&$%/": rejected
• ok 43 - invalid jspath "": rejected
• ok 44 - invalid jspath "1234": rejected
• ok 45 - iat claim accepted
• ok 46 - GET /rpc/get_guc_value: "0123456789abcdef"
• ok 47 - app.settings.name_var config reloaded with SIGUSR2
• ok 48 - jwt-secret config reloaded with SIGUSR2
• ok 49 - db-schemas config reloaded with SIGUSR2

For later PRs:

• make the postgrest endpoint dynamic/switch to unix domain sockets by default to allow parallelization
• include https://pypi.org/project/pytest-parallel/
• add docs for advanced usage (postgrest-watch postgrest-test-io -k ... etc.)

[wolfgangwalther]

Ah, the first test is already so much nicer compared to the bash script.

You might want to consider using pathlib. Specifically the following could make stuff even easier to read:

• https://docs.python.org/3/library/pathlib.html#operators
• https://docs.python.org/3/library/pathlib.html#pathlib.Path.iterdir
• https://docs.python.org/3/library/pathlib.html#pathlib.Path.read_text

[monacoremo]

You might want to consider using pathlib.

Yes, will be useful to handle the config files! I will also properly use pytest fixtures, avoiding the iteration in the config test function https://docs.pytest.org/en/stable/fixture.html#parametrizing-fixtures

[monacoremo]

I'll add more tests from the original bash file as I can, but probably not before next weekend - so marking as ready to review, please feel free to merge if you want to build on top of this scaffolding. When all tests are migrated, the can remove the old bash file.

[wolfgangwalther]

CI currently shows the tests as:

============================= test session starts ==============================
platform linux -- Python 3.8.5, pytest-5.4.3, py-1.9.0, pluggy-0.13.1
rootdir: /home/circleci/project
collecting ... collected 24 items                                                             

test/io-tests/test_io.py ........................                        [100%]

============================== 24 passed in 2.72s ==============================

Can we get an output with each test-case listed here as well? That would be more green :D

I'll add more tests from the original bash file as I can, but probably not before next weekend - so marking as ready to review, please feel free to merge if you want to build on top of this scaffolding. When all tests are migrated, the can remove the old bash file.

Looks very good so far. I don't think we need to hurry, but can merge this in one go when it's done. I have added another config io test to the bash script locally already - but that's written for now, so we can rewrite it when we're there.

[monacoremo]

Being able to filter the tests is going to be a great usability improvement I think - e.g. postgrest-watch postgrest-test-io-new -k reload

[monacoremo]

All tests covered! Now to clean it up, refactor and add some docs

[monacoremo]

Travis is acting up again, but we shouldn't have impacted anything that concerns it. Ready for review/merge :-)

[wolfgangwalther]

Travis is acting up again, but we shouldn't have impacted anything that concerns it.

Actually, we did. Travis runs the code-coverage currently and that includes the io-tests. They fail with:

Did not find executable at specified path: test/io-tests.sh

You could have a look at that and see how hard it would be to install those python deps there to be able to run them - or not. I wanted to move the code coverage to a nix tool and to circleci anyway, so that might be the time to do it...

[wolfgangwalther]

@monacoremo travis shot in the dark

Nice shot. I never make those on the first try. Nor the 2nd. :D

So I went through everything twice. Once just looking at the python stuff, once comparing the old io tests to the new ones. Everything is correct, only nit-picks and small improvements suggested.

Great job, Remo!

[monacoremo]

Nice shot. I never make those on the first try. Nor the 2nd. :D

I'm super surprised myself! :D 😎

Thank you for the great comments! Will add a few fixes

[wolfgangwalther]

Not sure if you already noticed: The reload tests fail kind of randomly. One test here, one test there. Maybe we need a sleep after the send_signal or something?

[monacoremo]

Yes, just noticed also - seems like we are going too fast. I added short sleeps in the affected tests

[wolfgangwalther]

Hmm. We still have an issue with the code coverage though. It's showing only 75%. Compare those two reports:

• master: https://coveralls.io/github/PostgREST/postgrest
• this pr: https://coveralls.io/builds/35672595

This PR is missing all the files that are collected by running the IO tests.

So it seems like the python IO test script is not launching the instrumented executable?

[wolfgangwalther]

Hm. Could be some stack issue as well. In the travis coverage job there is a huge debug output from stack - no idea what it means.

Later it also shows this:

  # merge the results from `stack test` and the io tests and exclude Paths_postgrest
  stack --no-terminal exec hpc -- sum --union --exclude=Paths_postgrest --output=/tmp/all.tix $_HPC_DIR/combined/all/all.tix test/io-tests/postgrest.tix

  # fix a bug in stack-hpc-coveralls
  mv $_MIX_DIR/hpc/Main.mix $_MIX_DIR/hpc/$_PKG_NAME/Main.mix
  mv $_MIX_DIR/hpc/UnixSocket.mix $_MIX_DIR/hpc/$_PKG_NAME/UnixSocket.mix
  sed -i -r "s/(Main|UnixSocket)/$_PKG_NAME\/\1/g" /tmp/all.tix

  # upload to coveralls
  mv /tmp/all.tix $_HPC_DIR/combined/all/all.tix
  shc combined all

hpc: user error (Pattern match failure in do expression at utils/hpc/HpcCombine.hs:127:3-14)
sed: can't read /tmp/all.tix: No such file or directory
mv: cannot stat '/tmp/all.tix': No such file or directory

This script is brittle - part of the reason why I want to switch to a different setup with nix etc.

The first command fails. I would assume it's because the test/io-tests/postgrest.tix file is not there. The simple reason could be that it's just in some other location now. The io-tests.sh script changed folder to test/io-tests - so that was the working directory.

The file is probably just in the repo root now.

[monacoremo]

So it seems like the python IO test script is not launching the instrumented executable?

It should also be using the postgrest executable on the path... It might have been an issue with passing the flags, let's see if this works

[wolfgangwalther]

I think this should do it:

stack --no-terminal exec hpc -- sum --union --exclude=Paths_postgrest --output=/tmp/all.tix $_HPC_DIR/combined/all/all.tix test/io-tests/postgrest.tix

to

stack --no-terminal exec hpc -- sum --union --exclude=Paths_postgrest --output=/tmp/all.tix $_HPC_DIR/combined/all/all.tix postgrest.tix

[monacoremo]

Yes, had the same thought!

...aand we're back randomly poking at travis again :D

[wolfgangwalther]

...aand we're back randomly poking at travis again :D

I'm glad about that. So much fun. Some things don't change.

[wolfgangwalther]

That worked. Even though the coverage is not shown now.. but that is a different bug. It's happening in other PRs as well, I just realized. Basically coverage is disabled right now...

At least this PR doesn't report a lower number, now. Just none at all, like the others :D

Fine for now.

[steve-chavez]

@monacoremo @wolfgangwalther Great work here guys! Liking the new python test suite.

One thing though. When running postgrest-test-io, I get a ReadTimeout error from time to time:

requests.exceptions.ReadTimeout: HTTPConnectionPool(host='127.0.0.1', port=49421): Read timed out. (read timeout=0.1)

/nix/store/cnp0f7y76izc2kpl6xfkmjwjq6i71cxd-python3-3.8.5-env/lib/python3.8/site-packages/requests/adapters.py:529: ReadTimeout

I also get a large stack trace, which I've pasted here: https://pastebin.com/bCqTbvbx

Could that be improved to make the test runs more deterministic?

[wolfgangwalther]

If I remember correctly, that would be the 1s timeout that is currently in place when waiting for postgrest to startup. Should probably just increase it. @monacoremo?