Allow to run tests with read-only credentials for DB

[GernotMaier]

closes #604

This PR changes the tests so that they can run with the read-only credentials for the DB:

• tests which require write access are marked with @pytest.mark.requires_db_admin

Note that the integration test tests/integration_tests/test_applications.py::test_applications[simulate_prod::gamma_20_deg_pack_for_grid] requires write access. We leave for the the write access for the integration tests.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (a5bd97a) 82.18% compared to head (989a41a) 82.10%.
Report is 46 commits behind head on main.

❗ Current head 989a41a differs from pull request most recent head 964b43d. Consider uploading reports for the commit 964b43d to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #711      +/-   ##
==========================================
- Coverage   82.18%   82.10%   -0.09%     
==========================================
  Files          42       42              
  Lines        6196     6224      +28     
==========================================
+ Hits         5092     5110      +18     
- Misses       1104     1114      +10     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[GernotMaier]

Obviously (!) tested only with read-only credentials. But tests in test_db_handler are skipped also for write credentials...

[GernotMaier]

This issue is fixed now. Tests are running correctly now depending on the DB configuration.

[orelgueta]

I am not sure about the status of this PR. Did you already set in the CI and everywhere the read-only user?

I would make one annoying suggestion, change the name of "admin" to "write" cause it's not really admin rights and we might even need that in the future.

Regarding the integration test. I suggest we mark that as requires_write for now because we anyway plan to split the applications into separate files/functions and then it will be easy to mark if needed (I really doubt we need it though, not sure what's going on there).

[GernotMaier]

Not ready for review (as it is draft :-) ). Waited for your answer in issue #604. So I do this tomorrow (or feel free to do it).

[GernotMaier]

@orelgueta - I think I need your help here.

units tests are almost all working individually (except test_db_handler.py::test_insert_files_db), but not when executed after each other. I don't see where I overwrite the db configuration from write to read.

[orelgueta]

Your solution in terms of pytest was fine, but you didn't take into account that the db_handler has its own protection against opening multiple connections to the DB.

Essentially, it was written such that once a connection is open, any other init of the DatabaseHandler would return the one already open. The idea was to protect against initialising the DatabaseHandler in a loop or in multiple classes and each time opening a new connection to the DB.

For the sake of this PR I removed that protection to allow two connections, one with read permissions and one with write permissions. Now we have to decide wether we think it is a good idea to remove the protection from DatabaseHandler for the sake of testing both connection types.

I tend to think it would be better to simply use the the read-only user in the production container tests instead of the solution in this PR. What do you think?

[GernotMaier]

This is definitely good - I was seriously in doubt about my understanding of pytest fixtures. I didn't know that there should be one DB instance only.

I agree with you that we should remove this protection.

What about the simple solution of running in the matrix of the unit tests one of the elements with write permissions? This would mix testing different python versions / pip vs mamba setup with db read / write setup, but would give us enough confidence that the code is good and tested.

Don't you think that some of the tools will also write to the database? Or did we agree (I would be fine with it) that tools produces output (in most cases files) and it is a separate step (as part of a workflow) to submit it to the database.

[orelgueta]

I agree with you that we should remove this protection.

I assume you meant to write "shouldn't", right?

What about the simple solution of running in the matrix of the unit tests one of the elements with write permissions? This would mix testing different python versions / pip vs mamba setup with db read / write setup, but would give us enough confidence that the code is good and tested.

Here as well, I assume you mean one of the elements with read permissions, right?
I think most elements in the matrix and local runs should use the write permission account so that the majority of the tests are comprehensive.
This can be done of course, it just requires adding "pytest marker decorators" to the tests that require write permissions.

However, since we are essentially limiting the scope of the read-only test to just checking that the user itself works (and not the code/tests), I don't see the benefit of running multiple tests with it instead of just one application/test. That is why I made the suggestion to run the 1-2 applications you plan to run in the prod container with the read-only account and that's it.

Don't you think that some of the tools will also write to the database? Or did we agree (I would be fine with it) that tools produces output (in most cases files) and it is a separate step (as part of a workflow) to submit it to the database.

I don't know yet if it will be part of a workflow or one single tool that also writes to the "production DB". Either way, when we get there, we can make sure that that application runs with the write permissions. For now, we can choose any application and run it with the read-only user.

[GernotMaier]

I agree with you that we should remove this protection.

I assume you meant to write "shouldn't", right?

shouldn't

What about the simple solution of running in the matrix of the unit tests one of the elements with write permissions? This would mix testing different python versions / pip vs mamba setup with db read / write setup, but would give us enough confidence that the code is good and tested.

Here as well, I assume you mean one of the elements with read permissions, right? I think most elements in the matrix and local runs should use the write permission account so that the majority of the tests are comprehensive. This can be done of course, it just requires adding "pytest marker decorators" to the tests that require write permissions.

I actually meant read permission. Most of the unit tests don't write to the DB (except test_db_handler.py). But we can also do it the other way around, very similar outcome. And yes, we would simply skip those tests then which require write permissions.

However, since we are essentially limiting the scope of the read-only test to just checking that the user itself works (and not the code/tests), I don't see the benefit of running multiple tests with it instead of just one application/test. That is why I made the suggestion to run the 1-2 applications you plan to run in the prod container with the read-only account and that's it.

Don't you think that some of the tools will also write to the database? Or did we agree (I would be fine with it) that tools produces output (in most cases files) and it is a separate step (as part of a workflow) to submit it to the database.

I don't know yet if it will be part of a workflow or one single tool that also writes to the "production DB". Either way, when we get there, we can make sure that that application runs with the write permissions. For now, we can choose any application and run it with the read-only user.

OK, all good - I think we agree.

[orelgueta]

Sorry, but did we agree to do?

• Add markers to the tests about writing permissions and run one matrix element without writing permissions?
• Or closing this PR without merging and instead running one application in the production container with read permissions?

[orelgueta]

After discussion on zoom we decided to close this PR without merging.
Instead of testing the read-only account in the unit tests or integrations tests, we will run one of the applications in the production container test using the read-only account. That should be enough in order to test the validity of that account.