Page MenuHomePhabricator

[queuing] Add an integration test between coordinator on instance and the daemon
ClosedPublic

Authored by johann on Wed, Nov 11, 4:52 PM.

Details

Summary

Run the dequeuer in the background and call submit run, then assert that the correct events occur

TODO: add to buildkite

Test Plan

run the test

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

johann added a reviewer: dgibson.
johann edited the summary of this revision. (Show Details)
integration_tests/test_suites/daemon-integration-test-suite/test_queued_run_coordinator.py
64 ↗(On Diff #25451)

What would be better here?

The k8s tests wait for the run coordinator pod to complete, we can't do the same with the daemon. We could switch to calling the method of the coordinator rather than actually spawning it in the background, though that would decrease coverage.

johann retitled this revision from [queuing] Add an integration test to [queuing] Add an integration test between coordinator on instance and the daemon.Thu, Nov 12, 4:44 PM
johann added a reviewer: alangenfeld.

This probably doesn't need to live under integration tests right? It could live with the other daemon tests

integration_tests/test_suites/daemon-integration-test-suite/test_queued_run_coordinator.py
65–71 ↗(On Diff #25545)

we can poll the runs db / event db (with a timeout)

This revision now requires changes to proceed.Thu, Nov 12, 6:41 PM

seems legit - ill let @dgibson do a final pass

some rote changes (particularly the contextmanager stuff), otherwise looks good

integration_tests/test_suites/daemon-integration-test-suite/test_queued_run_coordinator.py
17–28 ↗(On Diff #25565)

there isn't any reason for this to be a contextmanager right now

38–39 ↗(On Diff #25565)

this also does not need to be a contextmanager

63 ↗(On Diff #25565)

this, on the other hand, should be in a with statement so that it cleans up

69–77 ↗(On Diff #25565)

there is a poll_for_finished_run method you can use here

This revision is now accepted and ready to land.Fri, Nov 13, 3:11 PM
This revision was landed with ongoing or failed builds.Fri, Nov 13, 6:19 PM
This revision was automatically updated to reflect the committed changes.