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.

Sign up for GitHub

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

Jump to bottom

Give tests their own database #294

Merged
merged 7 commits into from
Jan 8, 2024
Merged

Give tests their own database #294

merged 7 commits into from
Jan 8, 2024

Conversation

raffomania
Copy link
Contributor

Fix #283

@raffomania raffomania self-assigned this Dec 5, 2023
@raffomania raffomania marked this pull request as ready for review December 5, 2023 15:37
@raffomania raffomania force-pushed the fix-database-in-tests branch 2 times, most recently from 64e1708 to aeedd5e Compare December 13, 2023 12:23
floschne
floschne requested changes Dec 14, 2023
View reviewed changes
Copy link
Member

@floschne floschne left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please resolve the conflicts :)

raffomania
raffomania commented Dec 15, 2023
View reviewed changes
backend/src/test/conftest.py
f"Database '{conf.postgres.db}' is not empty. The tests will only run given a database without any tables in it."
)
exit(1)

run_required_migrations()
startup(reset_data=True)
Copy link
Contributor Author

@raffomania raffomania Dec 15, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just noticed that this will wipe all project files, which is inconvenient when running the tests during development :(

I'd solve this by adding a call to mktemp in the new makefile, and setting REPO_ROOT to that folder.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in the makefile PR: 1628004

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

finde ich gut, d.h. aber dass wir Tests jetzt ausschließlich über das makefile starten sollten. wird beim makefile automatisch source .env; source .env.testing ausgeführt?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Guter Punkt, .env.testing hat noch gefehlt. Hab's jetzt eingefügt.

bigabig
bigabig reviewed Jan 8, 2024
View reviewed changes
.github/workflows/backend_check_schema.yml Outdated
@@ -9,6 +9,10 @@ on:
jobs:
check-schema:
runs-on: ubuntu-latest
env:
API_PRODUCTION_WORKERS: 0
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

was bedeutet denn API_PRODUCTION_WORKERS: 0? Muss das nicht mindestens 1 sein? Vorher war es auch auf 0 gesetzt, von daher ist es wahrscheinlich richtig so.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ich glaube, wir könnten den Wert aus der CI eigentlich ganz rausnehmen. Der Wert wird komplett ignoriert, wenn API_PRODUCTION_MODE nicht auf 1 gesetzt ist. Deswegen stehen da so sinnlose sachen wie 0 oder an anderer Stelle -10 drin.

@bigabig bigabig merged commit d18aff8 into mwp_v1 Jan 8, 2024
4 checks passed
@floschne floschne deleted the fix-database-in-tests branch January 8, 2024 16:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@floschne floschne floschne requested changes

@bigabig bigabig bigabig approved these changes

Assignees

@raffomania raffomania

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

reset_data does not reset the postgres database
3 participants
@raffomania @bigabig @floschne