-
Notifications
You must be signed in to change notification settings - Fork 1k
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
[22.05] Add missing celery dep to galaxy test base #15205
base: release_22.05
Are you sure you want to change the base?
[22.05] Add missing celery dep to galaxy test base #15205
Conversation
lib/galaxy_test/base/api.py
Outdated
@@ -58,8 +58,9 @@ def _request_celery_app(self, celery_session_app, celery_config): | |||
if os.environ.get("GALAXY_TEST_EXTERNAL") is None: | |||
from galaxy.celery import celery_app | |||
|
|||
celery_app.fork_pool.stop() | |||
celery_app.fork_pool.join(timeout=5) | |||
if celery_app.fork_pool: |
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.
This seems weird, are you bootstrapping your own instance or targeting an external 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.
This just subclasses integration test case: https://github.com/galaxyproject/total-perspective-vortex/blob/main/tests/test_mapper_resubmit.py
No other setup as such. Is anything else required?
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.
Hmm, I guess we'll have to do this, but maybe we should actually spin up a real, separate celery instance in the driver, so we can get rid of this.
Is it possible to move all of this to galaxy_test.driver? galaxy_test.base shouldn't have backend details like this in it. |
Co-authored-by: Marius van den Beek <[email protected]>
@mvdbeek Is this ok to be merged? It would help with getting the resubmission integration tests running for TPV. |
This is no longer an issue with packaged point releases, so I'll close this. |
I spoke to soon I think. This is still an issue: https://github.com/galaxyproject/total-perspective-vortex/actions/runs/6863066354/job/18662068049?pr=66 |
Co-authored-by: Nicola Soranzo <[email protected]>
LGTM. Do you still need to target 22.05 or would 23.0 be enough? |
@@ -42,6 +42,7 @@ install_requires = | |||
pytest | |||
PyYAML | |||
requests | |||
pytest-celery |
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.
Since commit 1fef07c we are not using pytest-celery
any more.
I was seeing this error as a result of the missing celery dependency:
It turned out that the
celery_session_app
fixture is coming from the celery app, which is not a declared dependency of galaxy_test_base.Fixing that results in the next error:
For which I've added the conditional stop. Why it's null in the first place, I don't know.
License