Page MenuHomePhabricator

Fix bad merge
ClosedPublic

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

Details

Summary

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

Test Plan

unit, tested dagit manually

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

natekupp 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
natekupp updated this revision to Diff 2817.Jul 12 2019, 4:13 PM

add test for the one, last untested line in handle.py

natekupp 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

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

This revision was automatically updated to reflect the committed changes.