test: Coodinator handlers #977
Conversation
.github/workflows/coordinator-ci.yml
Outdated
| @@ -21,6 +21,13 @@ jobs: | |||
| uses: arduino/setup-protoc@v2 | |||
| with: | |||
| repo-token: ${{ secrets.GITHUB_TOKEN }} | |||
| - name: Install Rust | |||
There was a problem hiding this comment.
Will conflict with #976 - I'll remove from whichever is merged first
| }; | ||
|
|
||
| handler | ||
| .synchronise(&config, Some(config.get_registry_version())) |
There was a problem hiding this comment.
I think the utility of having a function which encapsulates "Make the state match the action" is neat, it feels like it abstracts a lot of behavior. I think I would prefer if we had functions which described what they did. For example, "handle_update" or "handle_restart". So, the handler contains direct and concrete actions you can take with the resource, and lifecycle is fully responsible for calling them when appropriate. This removes the sharing of logical responsibility between the handler and lifecycle, where lifecycle seems to defer to the handler for things. What do you think?
Basically, migrate the synchronize logic to lifecycle.
There was a problem hiding this comment.
Yeah, I think I agree, but am also on the fence. On one hand, it's nice that the lifecycle is thin, and essentially manages state changes only. On the other, it encapsulates a lot like you've mentioned.
The encapsulation also allows us to refactor lifecycle quite easily, as the majority of the logic is delegated. This logic is pretty concrete and won't change. I've been burnt in the past having to refactor synchronisation many times, but with this encapsulation I can refactor around it.
I'd like to sit on this for a bit longer to really get a feel for what is/isn't working. I've also been burnt in the past pre-optimising. Are you ok with that?
There was a problem hiding this comment.
Yep! No issues with it. I'm happy to let it sit.
| } | ||
|
|
||
| #[tokio::test] | ||
| async fn restarts_unhealthy_streams() { |
There was a problem hiding this comment.
I added a pretty long wait between the stop and start call. Does this unit test also wait that timeout, or did you find a way to have it skipped?
There was a problem hiding this comment.
tokio::time::pause handles that
| } | ||
|
|
||
| #[tokio::test] | ||
| async fn restarts_unhealthy_executors() { |
There was a problem hiding this comment.
So is it here that we would change the test, once we add in limited retries?
There was a problem hiding this comment.
I think so, we'd expect it to stop after some amount of time.
morgsmccauley
force-pushed
the
test/coordinator-handlers
branch
from
8892aaa
to
0fb9321
Compare
Adds tests for:
DataLayerHandler,BlockStreamsHandler, andExecutorsHandler. No functional changes, but I did need to extract/create internal client wrappers so that the RPC requests can be mocked.