search

France-ioi / AlgoreaBackend

Backend for the new Algorea platform
MIT License
1 stars 2 forks source link

Make sure the tests cannot be run on live database as it empties the database #1085

Closed GeoffreyHuck closed 1 month ago

GeoffreyHuck commented 2 months ago

fixes #1080

An error is thrown if the test environment uses a database on AWS.

So the issue that happened last time wont happen anymore.

Works for Gherkin tests and also integration tests with a database.

Note: in the test TestDBConfig_FailsOnAWSForTestEnv, appenv.IsEnvTest has to be patch to return true for the test to succeed on CI. Which means the env is not called "test" on CI. Not sure if that's expected.

codecov[bot] commented 2 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 100.00%. Comparing base (a9f7dc3) to head (efc898f). Report is 40 commits behind head on master.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #1085 +/- ## ========================================= Coverage 100.00% 100.00% ========================================= Files 236 236 Lines 14321 14331 +10 ========================================= + Hits 14321 14331 +10 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

GeoffreyHuck commented 2 months ago

I investigated a little bit further:

  1. The env is not set to test when make test is used to execute the tests, like it is done on CI. I think it defaults to dev. This is most likely not intended.
  2. The env set to test when we execute the tests in the IDE (e.g. Jetbrains).

That's why it works on my machine but not on the CI.

The process that caused the issue

  1. Set the config DB to the live DB, to test a service
  2. Compile & Run the server
  3. Call the service I want by starting a curl to the server, which uses the live DB
  4. ... (working on something else and forgot to remove the live DB from the config)
  5. Run a test (in the IDE, so the test env is properly set
  6. ... (the first step of running a test with database is to clean the database)

This is the issue that this PR solves.

It would probably be nice to make sure the env is set to test when make test is called too.

Does this make sense to you?

smadbe commented 2 months ago

Wouldn't it make sense to force the env to be "test" (not force the to "test"!) when running test, and force the env not to be "test" when "serving". This would force to have 2 different config files, and force the choice (you could still keep one for default depending on what is your default operation).

GeoffreyHuck commented 1 month ago

Just found something interesting.

For the record, I've put all the config in config.yml and not config.test.yaml. I remembered it didn't work. I just found what happened: there was a syntax error in config.test.yaml, and so it wasn't used at all.

In the logs I had Cannot merge "test" config file, ignoring it: While parsing config: yaml: line 10: found unexpected end of stream.

So ended up setting everything is config.yml.

But even then, I don't think it's just a problem of env. I could have just though "let's start the server in test mode because I'm testing something" (AlgoreaBackend serve test), and would have updated the config.test.yaml with the live database, and then I would have faced the same problem. So even if we fix the env, we might run into this problem again in the future.

Maybe the kill switch should be in the test module itself when the tables are purged? Right before they are purged, we check if we are on aws? Or do you see another way of doing?

smadbe commented 1 month ago

For the record, I've put all the config in config.yml and not config.test.yaml. I remembered it didn't work. I just found what happened: there was a syntax error in config.test.yaml, and so it wasn't used at all.

In the logs I had Cannot merge "test" config file, ignoring it: While parsing config: yaml: line 10: found unexpected end of stream.

So ended up setting everything is config.yml.

Have you fixed the problem now?

But even then, I don't think it's just a problem of env. I could have just though "let's start the server in test mode because I'm testing something" (AlgoreaBackend serve test), and would have updated the config.test.yaml with the live database, and then I would have faced the same problem. So even if we fix the env, we might run into this problem again in the future.

That's why my suggestion was to forbid running serve in test env... that would not prevent you from copying the test config in the prod one. But at least the prod one could not be used to run test by mistake.

Maybe the kill switch should be in the test module itself when the tables are purged? Right before they are purged, we check if we are on aws? Or do you see another way of doing?

My initial point was that checking aws in the url is very vendor specific, if a partner wants to run it on Google cloud, the issue would come back.

GeoffreyHuck commented 1 month ago

Yes now it is fixed.

Okay I missed your point yesterday. That would work indeed. Forcing "test" for tests, and forbidding "test" for serve. Let's implement this!

GeoffreyHuck commented 1 month ago

It's done.

But I'm still not sure it's enough.

Nothing actually forbids to just use one config.yaml, and never set up a config.test.yaml.

smadbe commented 1 month ago

t's done.

But I'm still not sure it's enough.

Nothing actually forbids to just use one config.yaml, and never set up a config.test.yaml.

An option would be not to allow using the config.yaml file for tests (only loading config.test.yaml in this case)

GeoffreyHuck commented 1 month ago

Yeah, I'll also add a big warning comment in the config.test.sample.yml. I think it's acceptable.

So:

GeoffreyHuck commented 1 month ago

Done. The code is simple but the tests were a bit tricky to implement. I added comments to make it a bit clearer.