Page MenuHomePhabricator

Update test reqs and snapshots
ClosedPublic

Authored by max on Oct 20 2020, 6:27 PM.

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.Oct 20 2020, 6:55 PM
Harbormaster failed remote builds in B19862: Diff 24096!

update pytest everything

reverse direction of test directory helper

Harbormaster returned this revision to the author for changes because remote builds failed.Oct 20 2020, 10:42 PM
Harbormaster failed remote builds in B19889: Diff 24135!
Harbormaster returned this revision to the author for changes because remote builds failed.Oct 20 2020, 11:22 PM
Harbormaster failed remote builds in B19896: Diff 24145!
Harbormaster returned this revision to the author for changes because remote builds failed.Oct 21 2020, 12:41 AM
Harbormaster failed remote builds in B19902: Diff 24151!
Harbormaster returned this revision to the author for changes because remote builds failed.Oct 21 2020, 4:18 PM
Harbormaster failed remote builds in B19939: Diff 24192!
Harbormaster returned this revision to the author for changes because remote builds failed.Oct 21 2020, 7:29 PM
Harbormaster failed remote builds in B19955: Diff 24213!
Harbormaster returned this revision to the author for changes because remote builds failed.Oct 21 2020, 9:30 PM
Harbormaster failed remote builds in B19958: Diff 24217!
Harbormaster returned this revision to the author for changes because remote builds failed.Oct 21 2020, 10:23 PM
Harbormaster failed remote builds in B19963: Diff 24223!
Harbormaster returned this revision to the author for changes because remote builds failed.Oct 22 2020, 3:59 PM
Harbormaster failed remote builds in B19992: Diff 24259!

actually update coverage image

Harbormaster returned this revision to the author for changes because remote builds failed.Oct 22 2020, 10:17 PM
Harbormaster failed remote builds in B20039: Diff 24316!
max requested review of this revision.Oct 23 2020, 4:53 PM

tests do pass, the marked failure was a transient failure in a grpc test. in general i think that running using xdist will put pressure on our test suites -- tests which fail only under weird circumstances will fail more often. this is both good (in that edge cases that might appear under production loads may be more likely to surface) and bad (in that we may spend cycles refining test harnesses to solve issues that won't arise in prod)

i think that on average we might gain 1.5-2 minutes on the core test suites with this change

speeding up just the core tests doesn't effect BK total time - do you run tox locally? I always just pytest directly.

Honestly I will take this diff just for updating snapshots and deps - but will gives some time for others to chime in on xdist use itself

python_modules/dagster-graphql/dagster_graphql/cli.py
207

nit: extra spaces after API

python_modules/dagster/dagster/cli/api.py
338

^

dgibson added inline comments.
python_modules/dagster-graphql/dagster_graphql/cli.py
203

just confirming understanding here - we established that this only can actually happen in tests that use CliRunner.invoke right? (i.e. it's a test-specific issue - users shouldn't be synchronously executing command-line options in Python)

Generally it should be safe outside of tests to assume that a CLI command is on the main thread when it starts

This revision is now accepted and ready to land.Oct 26 2020, 1:32 PM

Are we sure dagster test suite is safe here? I know the migration tests were definitely historically not safe to run concurrently. This may be no longer be true.

The changes to the migration test setup here should make them safe to run concurrently (in python_modules/dagster/dagster/utils/test/__init__.py and the back_compat tests). In general, this would probably be more fragile than the single-threaded running, but like I said above, that's good and bad.

python_modules/dagster-graphql/dagster_graphql/cli.py
203

Tests are the only place I've been able to provoke this issue, but there is nothing stopping a user from invoking the command line functions in a child thread.

207

thx

I could break the snapshots, deps, and hygiene out into a separate diff and we can continue to discuss the risk of xdist separately if we prefer

Actually, going to disable xdist. Will land only the updated test reqs and snapshots

max retitled this revision from Try using pytest-xdist to Update test reqs and snapshots.Oct 27 2020, 5:42 PM
max edited the summary of this revision. (Show Details)
This revision was automatically updated to reflect the committed changes.