Page MenuHomePhabricator

Move dagster-celery-tests functions to utils and pipelines to repo.py
ClosedPublic

Authored by johann on Jul 30 2020, 3:36 PM.

Details

Summary

Move repeated functions into utils.
Move pipeline defs to separate repo.py. Since they are loaded via file, any relative imports would break in the test files otherwise.

Test Plan

Not great, a lot of these tests fail locally and are skipped in BK

Diff Detail

Repository
R1 dagster
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

Harbormaster returned this revision to the author for changes because remote builds failed.Jul 30 2020, 4:36 PM
Harbormaster failed remote builds in B16222: Diff 19806!
Harbormaster returned this revision to the author for changes because remote builds failed.Jul 30 2020, 7:54 PM
Harbormaster failed remote builds in B16238: Diff 19825!

move execute_pipeline_on_celery to util

Harbormaster returned this revision to the author for changes because remote builds failed.Jul 31 2020, 6:03 PM
Harbormaster failed remote builds in B16276: Diff 19871!
johann retitled this revision from clean up celery tests to Move dagster-celery-tests functions to utils.Jul 31 2020, 6:53 PM
johann edited the summary of this revision. (Show Details)
johann edited the test plan for this revision. (Show Details)
johann added reviewers: catherinewu, nate.
Harbormaster returned this revision to the author for changes because remote builds failed.Jul 31 2020, 7:53 PM
Harbormaster failed remote builds in B16292: Diff 19889!
johann retitled this revision from Move dagster-celery-tests functions to utils to Move dagster-celery-tests functions to utils and pipelines to repo.py.Jul 31 2020, 8:26 PM
johann edited the summary of this revision. (Show Details)
Harbormaster returned this revision to the author for changes because remote builds failed.Jul 31 2020, 8:38 PM
Harbormaster failed remote builds in B16300: Diff 19900!
Harbormaster failed remote builds in B16301: Diff 19901!

This should be good for review, the failing suites are the broken sqlite and flyte tests.

python_modules/libraries/dagster-celery/dagster_celery_tests/utils.py
31

i think this test is only run if its in a file with name "test_.*"?

python_modules/libraries/dagster-celery/dagster_celery_tests/utils.py
45

curious about introduction of "tempdir_wrapper" instead of calling "seven.TemporaryDirectory()" directly?

johann marked an inline comment as done.

rename utils

python_modules/libraries/dagster-celery/dagster_celery_tests/utils.py
31

Good catch, thanks

45

I just wanted a way to conditionally use a new temp dir unless one was already provided (the test cases use a mix of both). Would it be cleaner to just have them pass in the instance they created rather than getting it from the same temp file?

movingshitaround

python_modules/libraries/dagster-celery/dagster_celery_tests/repo.py
19

not a fan of committing TODO comments. Delete or add link to GH issue with sufficient explanation

python_modules/libraries/dagster-celery/dagster_celery_tests/test_utils.py
1

maybe just call this file utils since it doesn't contain any tests, and in the test folder test_ prefix means tests

This revision is now accepted and ready to land.Aug 3 2020, 8:37 PM