Page MenuHomeElementl

Separate interval for check step health
ClosedPublic

Authored by johann on Jun 3 2021, 3:07 PM.

Diff Detail

Repository
R1 dagster
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

johann added inline comments.
python_modules/dagster/dagster_tests/core_tests/engine_tests/test_step_delegating_executor.py
163

Don't love this test setup if you have suggestions

johann requested review of this revision.Jun 3 2021, 3:49 PM

leaving open for a bit for the question for jordan

python_modules/dagster/dagster/core/executor/step_delegating/step_delegating_executor.py
25โ€“26

is there still a real reason a user would want to set this at this point?

python_modules/dagster/dagster_tests/core_tests/engine_tests/test_step_delegating_executor.py
152

0 essentially means "on every sleep"? (related to question above about whether a user would realistically set sleep_seconds at this point)

Feels a little unintuitive, maybe the executor should assert that check_step_health_interval_seconds is >= sleep_seconds?

163

@jordansanders do you know if there's a good way to use mocks to get method calls without stubbing out the implementation?

python_modules/dagster/dagster/core/executor/step_delegating/step_delegating_executor.py
25โ€“26

Probably not, but it's not really a user facing config anyway unless it gets added to the executor definitions

python_modules/dagster/dagster_tests/core_tests/engine_tests/test_step_delegating_executor.py
152

I don't have a strong opinion, but I don't imagine executors generally exposing this config. Added it here for tests, but I think real executors can leave it to the defaults.

python_modules/dagster/dagster_tests/core_tests/engine_tests/test_step_delegating_executor.py
163

I actually think this is quite a nice approach to testing the particular assertions you want to make - you've basically written a spy here. Sort of like what I did here: https://github.com/dagster-io/internal/commit/5b180d4fabb704273d97daaa5f69e49a2adfc30d#diff-30f14aa7a85ae4f5864b513197ab0c38a6217901faaaf98667ce330a2e0d2bca

In general, I prefer writing fake implementations to using mocking libraries because I think the former is easier to understand.

Although one major risk with any type of test doubles is that you end up tightly coupling to implementation details (methods x, y, z were called in that order) instead of testing behavior. Is the number of times it calls each method actually a behavior we care about? Or is it just an implementation detail?

python_modules/dagster/dagster_tests/core_tests/engine_tests/test_step_delegating_executor.py
163

Although one major risk with any type of test doubles is that you end up tightly coupling to implementation details (methods x, y, z were called in that order) instead of testing behavior... is the number of times it calls each method actually a behavior we care about?

Agree with the concern. I think here the two objects are pretty decoupled (step delegating executor and the step handlers), so testing the step delegating executor by how it interacts with the step handler's methods seems appropriate

python_modules/dagster/dagster_tests/core_tests/engine_tests/test_step_delegating_executor.py
163

Better stated, these aren't just internal helper methods, they're a defined api boundary so I think it's more acceptable if changing them breaks tests

seems reasonable to me then

This revision is now accepted and ready to land.Jun 3 2021, 6:18 PM
This revision was automatically updated to reflect the committed changes.