Page MenuHomePhabricator

Fix bad merge

Authored by nate on Jul 12 2019, 4:01 PM.



Had a bad merge in D582 that reverted Alex’s fix in D579. D582 added a bunch of tests for, and left only a single line untested -- which happened to be this one.

Test Plan

unit, tested dagit manually

Diff Detail

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

Event Timeline

nate created this revision.Jul 12 2019, 4:01 PM
alangenfeld accepted this revision.Jul 12 2019, 4:03 PM

did the tests we talked about get added?

This revision is now accepted and ready to land.Jul 12 2019, 4:03 PM
nate updated this revision to Diff 2817.Jul 12 2019, 4:13 PM

add test for the one, last untested line in

nate edited the summary of this revision. (Show Details)Jul 12 2019, 4:18 PM

Seems like we should have a layer of tests at the cli level too. i.e. actually invoke our utilities with -f -n combos to ensure that they work

btw not suggesting blocking this diff for those tests, we should land this

nate added a comment.Jul 12 2019, 4:48 PM

Yeah looking now, python_modules/dagit/dagit_tests/ is pretty light - will open an issue to track

This revision was automatically updated to reflect the committed changes.