-
Notifications
You must be signed in to change notification settings - Fork 218
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
Convert unittest to pytest #3622
Conversation
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.
Looks really good to me, I just have a couple questions for clarification, but nothing is necessarily a blocker 👍
catalog/tests/dags/common/sensors/test_single_run_external_dags_sensor.py
Show resolved
Hide resolved
data. | ||
""" | ||
|
||
es = _get_es() |
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.
Maybe _get_es
could also be converted to a fixture? It's used as such, but I don't know what the specific benefit would be either way, other than that it looks like a fixture to me.
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.
Yes, I have converted _get_es
to sample_es
as a fixture.
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.
LGTM! In looking at the ingestion server tests, I noticed this warning: https://github.com/WordPress/openverse/actions/runs/7405567531/job/20148753309?pr=3622#step:5:31
I was going to open an issue, but saw this comment on the code from @dhruvkb talking about using Bottle instead of Falcon:
Dhruv: Do you remember what the issue with Falcon was? Can you open an issue describing it? It would be nice to find a workaround to use Falcon instead or to find an alternative to Bottle, less so because of the deprecation warning and more to simplify the dependencies (which in turn means we also have fewer moving parts to worry about deprecations).
Anyway, that's a separate issue from this PR, I just wanted to bring it up for Dhruv to look at to make sure we have it recorded just in case it's an issue in the future.
catalog/tests/dags/common/sensors/test_single_run_external_dags_sensor.py
Outdated
Show resolved
Hide resolved
…s_sensor.py Co-authored-by: Staci Mullins <63313398+stacimc@users.noreply.github.com>
Based on the low urgency of this PR, the following reviewers are being gently reminded to review this PR: @krysal Excluding weekend1 days, this PR was ready for review 5 day(s) ago. PRs labelled with low urgency are expected to be reviewed within 5 weekday(s)2. @ngken0995, if this PR is not ready for a review, please draft it to prevent reviewers from getting further unnecessary pings. Footnotes
|
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.
Great work, @ngken0995! Thank you!
Co-authored-by: Krystle Salazar <krystle.salazar@automattic.com>
Fixes
Fixes #3425 by @AetherUnbound
Description
On
catalog/tests/dags/common/sensors/test_single_run_external_dags_sensor.py
,setUp
function is renamed tosetup_pool
,set_pool
will have a parametersample_pool_fixture
retrieved from conftest.TestExternalDAGsSensor
class will be removed and all test function will be in the same module.On
ingestion_server/test/integration_test.py
,setUpClass
is rename tosetup_fixture
,setup_fixture
use scope as module, autouse is set as True and it will return a dictionary for other function to retrieve them. Installpytest-subtests
to replace unittestsubTest
.Testing Instructions
For catalog:
run
just catalog/test
For ingestion server:
if ingestion server fail:
run
just ingestion_server/test-clean-dc
run
just ingestion_server/test_local
Checklist
Update index.md
).main
) or a parent feature branch.Developer Certificate of Origin
Developer Certificate of Origin