Page MenuHomePhabricator

parallelize dagster_tests/
AbandonedPublic

Authored by aj.nadel on Jul 2 2020, 12:12 AM.

Details

Reviewers
sashank
nate
Summary

Addresses https://github.com/dagster-io/dagster/issues/2641

Gives api_tests/, cli_tests/, core_tests/ their own tox envs, putting the rest under the -general suffix.
Adds these suffices to the corresponding Buildkite pipeline step.

Test Plan

bk (pipeline changes, tox changes)

Diff Detail

Repository
R1 dagster
Branch
buildkite-pole-shortening
Lint
Lint OK
Unit
No Unit Test Coverage

Event Timeline

aj.nadel created this revision.Jul 2 2020, 12:12 AM
Harbormaster returned this revision to the author for changes because remote builds failed.Jul 2 2020, 12:25 AM
Harbormaster failed remote builds in B14569: Diff 17867!
aj.nadel updated this revision to Diff 17882.Jul 2 2020, 1:05 AM

fixed syntax in toxfile

aj.nadel requested review of this revision.Jul 2 2020, 1:18 AM
aj.nadel retitled this revision from split up dagster tests into parallel groups to parallelize dagster_tests/.Jul 2 2020, 1:30 AM
aj.nadel edited the summary of this revision. (Show Details)
aj.nadel edited the test plan for this revision. (Show Details)
aj.nadel added a reviewer: sashank.
aj.nadel added inline comments.Jul 2 2020, 1:38 AM
python_modules/dagster/tox.ini
24

If there's a nicer way to bundle these ignores together, I'd love to make use of it. But I don't see anything reading through the tox docs that seems immediately useful...

sashank added a reviewer: nate.Jul 2 2020, 2:09 AM
nate added a comment.Jul 2 2020, 3:26 PM

hmm, can you just move all of the other dagster tests under dagster_tests/general_tests? then you shouldn't need the ignores, right?

hmm, can you just move all of the other dagster tests under dagster_tests/general_tests? then you shouldn't need the ignores, right?

+1 to this. The only thing we have to be careful about is thinking about tests people add outside one of these folders. They might not realize that they're not being run by BK.

aj.nadel updated this revision to Diff 17935.Jul 2 2020, 7:24 PM

move general tests into one dir to simplify tox.ini

aj.nadel updated this revision to Diff 17954.Jul 2 2020, 8:53 PM

fix up some broken imports / paths

nate accepted this revision.Jul 5 2020, 5:41 PM

This looks good to me—please give the team a heads up when you land this, since everyone will want to rebase to pick up the moved tests

This revision is now accepted and ready to land.Jul 5 2020, 5:41 PM
sashank requested changes to this revision.Jul 6 2020, 3:49 PM

Looks great! I’d recommend creating a new diff that just moves the tests in to /general_tests, and rebasing this off of that. There have been quite a few tests modified in dagster_tests, and the merge conflict would be unnecessarily complicated to work through.

Also make sure none of the .db, .db-shm, and .db-wal files are committed, there shouldn’t be any changes in them.

This revision now requires changes to proceed.Jul 6 2020, 3:49 PM
aj.nadel abandoned this revision.Jul 7 2020, 9:28 PM

Closed in favor of D3771 and D3775