-
Notifications
You must be signed in to change notification settings - Fork 2
feat: Separate task processor database #68
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
base: main
Are you sure you want to change the base?
Conversation
@pytest.fixture | ||
def task_processor_database_tasks( | ||
mocker: MockerFixture, | ||
settings: SettingsWrapper, | ||
) -> Generator[Callable[..., list[Task]], None, None]: | ||
""" | ||
Prepare a fake tasks processor database | ||
""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not create an actual one?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good question. We'd need to have a separate test suite, in order to setup a separate database — or something else. I agree it would provide us an improved functional test, but it comes with some drawbacks. I figured we could get away the way it is because:
- The actual task fetching is tested against the default database.
- The effort of setting up a new database could lead us to test Django stuff.
Still, I'm not 100% convinced myself. WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As discussed offline, refer to how analytics database is handled in the container entrypoint and tests:
https://github.com/Flagsmith/flagsmith/blob/7f0035e70b548414661c1230be9ab6057d3c454c/api/scripts/run-docker.sh#L56-L62
https://github.com/Flagsmith/flagsmith/blob/7f0035e70b548414661c1230be9ab6057d3c454c/.github/workflows/api-pull-request.yml#L59-L63
https://github.com/Flagsmith/flagsmith/blob/7f0035e70b548414661c1230be9ab6057d3c454c/api/tests/unit/app_analytics/test_analytics_db_service.py#L48-L53
81192cf
to
0607086
Compare
Ref: Flagsmith/flagsmith#5136
WIP