Use pytest without unittest's TestCase class

[eloquence]

Status

Ready for review

Fixes #152

Description

Switch to pytest

We were previously inheriting from unittest's TestCase class, which did not convey obvious benefits but conflicted with the use of pytest features like parametrization:

unittest.TestCase methods cannot directly receive fixture arguments as implementing that is likely to inflict on the ability to run general unittest.TestCase test suites. source

This PR avoids inheriting from TestCase, and switches us to the leaner pytest approach to validating assertions. We can now also use pytest decorators like pytest.mark.parametrize (though we'll still have to be careful regarding interaction with VCR cassettes, which is why I've not done so here yet).

Simplifications of API proxy tests

• Rely on datasaver to provide API token
• Avoids TOTP retry logic in proxy tests. Without a way to exclude sleeps from the cassettes, there's very little value in automatically retrying TOTP codes (otherwise the sleeps will also be replayed)
• Removes logout.txt logic as we currently don't need it
• For readability, renamed decorator method to qrexec_datasaver

Clearer error handling when proxy is missing or malfunctioning

If the proxy package is not installed in the target VM, the SDK would previously just fail with "Error parsing JSON". We now raise a more specific error message if we get no response on stdout.

Test ordering

• Clarified which tests currently have ordering effects, and moved some create/destroy operations to the end to minimize those effects.

New configuration file location

• The config file is now in the private volume. Clarified this in the README.

Testing

• Attempt to re-generate JSON and YML files by following the instructions in the revised README. Are you able to reliably run the test suite? Do the cassette recordings match your expectations for a given test? Do the requests you see to your SecureDrop Core development environment match what you're expect on initial run and playback?

• Attempt to immediately regenerate JSON recordings after having done so once, to trigger a TOTP authentication failure. Is the error message clear and sufficient?

[eloquence]

I've moved this to "Ready for review", but it's still a draft; I've observed some odd behavior where it sometimes re-fetches a token when it doesn't need to, which I'd like to debug a bit more.

I'd appreciate some initial comments on the changes/simplifications, especially to the API proxy tests.

[sssoleileraaa]

This PR avoids inheriting from TestCase, and switches us to the leaner pytest approach to validating assertions. We can now also use pytest decorators like pytest.mark.parametrize (though we'll still have to be careful regarding interaction with VCR cassettes, which is why I've not done so here yet).

Are you saying we have to be careful here because we will generate a cassette for the run of the test with the first parameter and then skip cassette generation for runs with the second and any other following parameters?

[eloquence]

Are you saying we have to be careful here because we will generate a cassette for the run of the test with the first parameter and then skip cassette generation for runs with the second and any other following parameters?

Skip, or overwrite - either is bad if we expect different API response data for differently parametrized tests. Ideally, I think, we'd be able to stash the result for differently parametrized tests as separate cassettes, each with a unique filename.

So if you have a test_fruit that is parametrized to check for apple and orange responses, you could have test_fruit-apples.json and test_fruit-oranges.json (or YML) instead of writing to the same cassette.

[sssoleileraaa]

Gotcha. Well so far code review looks good so tomorrow I'll run though cassette generation on qubes following the docs to make sure all goes smoothly.

[eloquence]

CI will fail until freedomofpress/securedrop#5667 is merged.

[sssoleileraaa]

things are looking good but the readme instructions no longer work around regenerating cassettes so that just needs an update.

[sssoleileraaa]

nit: can we call this setup instead of setup_method?

[eloquence]

This method name is designated by pytest for the setup that runs before each test in class-based tests:
https://docs.pytest.org/en/stable/xunit_setup.html#method-and-function-level-setup-teardown

[sssoleileraaa]

💎

[sssoleileraaa]

adding the datasaver decorator here makes it so that the readme instructions are no longer accurate. on main you'll see that you can follow these steps from the readme:

2. [Skip if adding a new test] Delete the cassettes you wish to regenerate or just delete all json files by running:

   rm data/*.json

3. Comment out the @qrexec_datasaver decorator above the test you want to generate a new cassette for or just generate all new cassettes by commenting out the decorator above all methods in the test_apiproxy.py::TestAPIProxy class.

4. Make qrexec calls to the server and collect real response data:

   make test TESTS=tests/test_apiproxy.py

Perhaps this no longer works because of adding the decorator here?

[eloquence]

Good catch! I think it's because we're no longer conditionally loading a recorded token based on the presence of a testtoken.json file on disk, which means that re-running an individual test without commenting out qrexec_datasaver for setup_method will not work (because it'll replay the last token during that test's setup). Will verify and clarify.

[sssoleileraaa]

was this never necessary?

[sssoleileraaa]

discussed out-of-band some ideas about making tests less brittle so that we can run them in random order in the future. for now, since the logout test will be the last test to run, this code is fine to remove

[sssoleileraaa]

Thanks for taking the time to understand the nitty gritty of how this works. The code is easier to read and cassette generation is faster now that you've removed the unnecessary step of commenting out the datasaver decorator.

🚢