Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add/data status update #119

Merged
merged 4 commits into from
Dec 1, 2023
Merged

Add/data status update #119

merged 4 commits into from
Dec 1, 2023

Conversation

PGijsbers
Copy link
Contributor

@PGijsbers PGijsbers commented Nov 30, 2023

There is currently no "migration test" (=verifying the PHP response is identical to the Python response, minus documented changes), because the test infrastructure makes this hard. Since the PHP calls directly
mutate the database, we need to ensure we can recover the old database state after the test. We can not generally do this for the dataset status update call, as for that we need to know the initial state of the database (e.g., activating a dataset can be achieved by either adding an active row or removing a deactivated row).
While we do know the database state, we would need to hard-code that information into the tests. A better approach would be to start up a new separate database container for PHP (from the same image). This would negate the need to "undo".

A second issue that arises because of ordering:

  • if we call PHP first, the database is changed and the Python response would be different
  • if we call Python first, it opens a transaction for the duration of the test, which blocks execution of the query issued from PHP, which halts the test.

This too is mitigated by introducing a second database. The only potential risk you introduce is to be working on different databases. Overall though, the effect of this should be rather minimal as the database would be effectively reset after every individual test.

Spinning up an instance for each test does likely add significant overhead. I would have to measure, but I would expect 1~2 seconds per test locally.

The second database could also just be a SQLite database. At least for this test, the required tables and populated data are manageable and hopefully is much faster than starting a container. Though I think it might not work as neatly when we requiring different data across many tables (though if neatly organized into different fixtures, maybe that works OK).

There is currently no migration test because the test infra-
structure makes this hard. Since the PHP calls directly
affect the database, we need to ensure we can recover the old
database state after the test. We can not generally do this
for the dataset status update call, as for that we need to know
the initial state of the database. While we do know that tech-
nically, we would need to hard-code that information into the
tests. A better approach would be to start up a new database
container for PHP. A second issue that arises it that if we
call an update on the status table from both Python and PHP
then the transaction from the first will block the call of
the other. This too is mitigated by introducing a second
database. The only potential risk you introduce is to
be working on different databases. Overall though, the effect
of this should be rather minimal as the database would be
effectively reset after every individual test.
@PGijsbers PGijsbers merged commit 4a7f104 into main Dec 1, 2023
3 checks passed
@PGijsbers PGijsbers deleted the add/data_status_update branch December 1, 2023 08:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant