canonical / ubuntu-com-security-api

The API for CVEs and USNs data.
17 stars 9 forks source link

Rewrite tests to use a real postgresql database #85

Closed nottrobin closed 2 years ago

nottrobin commented 2 years ago

Before this, tests were using alchemy-mock. This is a very superficial mocking library which effectively just provides methods for mocking the return values for specific methods on a session object (e.g. filter).

The most immediate problem we ran into was that a trivial change in the code, e.g. from query.filter_by(id).one_or_none() to query.get(id) would break the test, which it really shouldn't as nothing meaningful has changed.

However, a much deeper issue is that a fair amount of the logic of this API exists within its data model. If we're not testing the way data is related within SQLAlchemy, we are probably missing the main areas where mistakes will be made.

Given this requitement, it seems clear that the best way to test will be with a real database. Since this is perfectly possible within each of our test environments without adding external dependencies, I feel this is the way to go.

It also appears to be [a not uncommon practice][2].

So here we do the following steps:

1: https://pypi.org/project/alchemy-mock/ 2: https://flask-testing.readthedocs.io/en/latest/#testing-with-sqlalchemy

QA

First, check tests pass on this PR.

Then also make sure the test pass locally:

dotrun test-python

I guess you might as well check the site actually works as well ;)

dotrun
mtruj013 commented 2 years ago

@nottrobin So both dotrun test-python and dotrun run successsfully, but for some reason the endpoint for getting all cves is erroring out. I tested the other get endpoints and they all work fine, though. Here's the error: AssertionError: Dependency rule tried to blank-out primary key column 'status.cve_id' on instance '<Status at 0xffff9924cd90>'

nottrobin commented 2 years ago

@mtruj013 thanks, I'd left in a change to the model from before when I was experimenting which I'd forgotten about. I've removed it now (and tidied up some of my old commits). Could you re-review?