Remove test_mode

[ThomasLaPiana]

Closes #1175

Code Changes

• [x] remove test_mode and various assertions from everywhere
• [ ] set pytest to bring up and tear down a test database entirely for each run using the test database config values

Steps to Confirm

• [ ] list any manual steps taken to confirm the changes

Pre-Merge Checklist

• [ ] All CI Pipelines Succeeded
• Documentation Updated:
  • [ ] documentation complete, or draft/outline provided (tag docs-team to complete/review on this branch)
  • [ ] documentation issue created (tag docs-team to complete issue separately)
• [ ] Issue Requirements are Met
• [ ] Relevant Follow-Up Issues Created
• [ ] Update CHANGELOG.md

Description Of Changes

Having an explicit test mode added complexity and didn't feel like a great solution. Instead, this PR relies on dev_mode where needed to replace test_mode and makes the much larger change of updating pytest to bring up and tear down its own separate database.

[ThomasLaPiana]

migrations aren't working now, getting an error around test mode even though it doesn't get mentioned anywhere, will keep investigating

[ThomasLaPiana]

I found the culprit in fideslib, luckily it looks like I can pass a database URI instead and bypass the issue

[ThomasLaPiana]

@pattisdr curious what your ideas are for a problem I'm hitting here technically:

In ctl we start the webserver, and then run tests against it directly via the API. As far as I can think of, we also have no way to modify the database it is connected to after it starts up. The webserver does its own database setup at startup, so I'm struggling to think of a way to modify the code so that we can inject a new database into the application at runtime without reworking a ton of code (or even if we did rework a ton of code). Do you have any ideas here?

The only thing that I can think of would be to start another instance of the webserver that is pointing at the test database?

[pattisdr]

@seanpreston do you know what we did in Fidesops for this? I guess I don't really understand how we fundamentally ran tests over there.

[ThomasLaPiana]

When I looked through it, it looks like ops is getting a database session directly, instead of going through the API for most cases

In the cases where it is using the api_client, I'm not sure how it wouldn't be modifying the application database itself

[pattisdr]

Ah OK, tracing it through

• api_client uses FastAPI's TestClient and we pass the fidesops application to it
• when an endpoint is hit in the test, get_db is injected
• that was eventually calling get_db_session in fideslib
• which called get_db_engine which checked if config.is_test_mode

[ThomasLaPiana]

Ah OK, tracing it through

• api_client uses FastAPI's TestClient and we pass the fidesops application to it
• when an endpoint is hit in the test, get_db is injected
• that was eventually calling get_db_session in fideslib
• which called get_db_engine which checked if config.is_test_mode

Thanks for tracing it!

Ah I see, so the application itself was aware of test mode...

I guess we have a few options here then:

1. Get rid of test mode as planned, but lose the ability to run tests against a separate database (which ctl can't do anyway)
2. Keep test_mode and update ctl to follow the ops solution. Ctl doesn't use dependency injection though so a (semi) significant refactor would be needed
3. Since the tests are destructive as they are now (no separate database for the ctl tests at least), we can remove test mode/the concept of a test database altogether and make sure that tests don't run unless dev_mode is active.

Do you see any other potential solutions here? Are you leaning towards any in particular?

I don't see any way to really satisfy the original ticket, since it seems like we get to either drop test mode or have separate application and test databases, but maybe I'm not thinking of a clever enough solution

[pattisdr]

My main motivation was having a separate application and test database! Getting rid of test mode I thought was just a means to that end, to help unify the ctl and ops pieces, but that doesn't seem to be true.

The current setup is making it difficult to develop ops-related code. When I'm switching between writing tests and running things locally, I keep finding the database getting wiped because they're sharing the same location. I think this is a deterrent to running the app locally when we should be doing this more not less.

So when you lay out those options, I have a strong preference for #2

[seanpreston]

The current setup is making it difficult to develop ops-related code. When I'm switching between writing tests and running things locally, I keep finding the database getting wiped because they're sharing the same location. I think this is a deterrent to running the app locally when we should be doing this more not less.

This is the reason we had the tests hit a separate DB to the running application's — if you want to run tests and develop at the same time it's often good for productivity so we should support it.

Ctl doesn't use dependency injection though so a (semi) significant refactor would be needed

@ThomasLaPiana — I'd be interested to see if there's a way we can dynamically change the DB connection URL based on a config var? At it's heart that's all the ops solution does.

[ThomasLaPiana]

The current setup is making it difficult to develop ops-related code. When I'm switching between writing tests and running things locally, I keep finding the database getting wiped because they're sharing the same location. I think this is a deterrent to running the app locally when we should be doing this more not less.

This is the reason we had the tests hit a separate DB to the running application's — if you want to run tests and develop at the same time it's often good for productivity so we should support it.

Ctl doesn't use dependency injection though so a (semi) significant refactor would be needed

@ThomasLaPiana — I'd be interested to see if there's a way we can dynamically change the DB connection URL based on a config var? At it's heart that's all the ops solution does.

Where there's a will there's a way! But I'd have to spend some time thinking about it, and probably look more at the ops solution to get some ideas. I just don't have the bandwidth for this right now, but I agree it's something we should do in the near-term to keep everyone productive. I also welcome anyone who wants to/has ideas to take a pass at it

[ThomasLaPiana]

i'm going to update the issue this is related to and then close this PR, since the strategy will be totally different