feat(protocol-engine): Implement CommandState.get_next_request()
#7936
Conversation
Codecov Report
@@ Coverage Diff @@
## edge #7936 +/- ##
==========================================
+ Coverage 83.78% 83.93% +0.14%
==========================================
Files 352 355 +3
Lines 21627 21895 +268
==========================================
+ Hits 18120 18377 +257
- Misses 3507 3518 +11
Continue to review full report at Codecov.
|
SyntaxColoring
added
robot-svcs
api/tests/opentrons/protocol_engine/state/test_command_state.py
Outdated
Show resolved
Hide resolved
api/tests/opentrons/protocol_engine/state/test_command_state.py
Outdated
Show resolved
Hide resolved
api/tests/opentrons/protocol_engine/state/test_command_state.py
Outdated
Show resolved
Hide resolved
| created_at=now, | ||
| request=r | ||
| ) | ||
| for r in _make_unique_requests(3) |
There was a problem hiding this comment.
I think this fixture function might be doing a little too much. Is this easier to read as an unrolled loop, without using actual commands?
command_a = PendingCommand(
created_at=datetime(year=2021, month=1, day=1),
request=BaseModel()
)
command_b = PendingCommand(
created_at=datetime(year=2022, month=2, day=2),
request=BaseModel()
)
command_c = PendingCommand(
created_at=datetime(year=2023, month=3, day=3),
request=BaseModel()
)
store.handle_command(command_a, "command-id-1") # type: ignore[arg-type]
store.handle_command(command_b, "command-id-2") # type: ignore[arg-type]
assert store.commands.get_all_commands() == [
("command-id-1", command_a),
("command-id-2", command_b),
]
store.handle_command(command_c, "command-id-1") # type: ignore[arg-type]
assert store.commands.get_all_commands() == [
("command-id-1", command_c),
("command-id-2", command_b),
]There was a problem hiding this comment.
I think I like rolling the loop into a function because it lets us name which trait in this triplet of commands is important. For example, unrolling the loop means we have to write PendingCommand (I think?), which feels overly specific. We care that they're unique, and nothing else.
Also, at the very least, I should simplify the usage to something like:
[command_a, command_b, command_c] = _make_unique_commands(3)There was a problem hiding this comment.
What if you used the other value fixtures?
def test_command_state_preserves_handle_order(
pending_command: PendingCommand,
running_command: RunningCommand,
completed_command: CompletedCommand,
) -> None:
store.handle_command(running_command, "command-id-1")
store.handle_command(pending_command, "command-id-2")
assert store.commands.get_all_commands() == [
("command-id-1", running_command),
("command-id-2", pending_command),
]
store.handle_command(completed_command, "command-id-1")
assert store.commands.get_all_commands() == [
("command-id-1", completed_command),
("command-id-2", pending_command),
]There was a problem hiding this comment.
Definitely better, but ultimately I think I have the same objection. pending_command, running_command, and completed_command seem lower than the most meaningful level of abstraction for this particular test.
For now, I'm moving forward with your suggestion, plus a comment pointing out that the commands are just an arbitrary triplet that compares != to each other.
api/tests/opentrons/protocol_engine/state/test_command_state.py
Outdated
Show resolved
Hide resolved
api/tests/opentrons/protocol_engine/state/test_command_state.py
Outdated
Show resolved
Hide resolved
api/tests/opentrons/protocol_engine/state/test_command_state.py
Outdated
Show resolved
Hide resolved
| @@ -27,3 +54,110 @@ def test_state_store_handles_command(store: StateStore, now: datetime) -> None: | |||
| store.handle_command(cmd, command_id="unique-id") | |||
|
|
|||
| assert store.commands.get_command_by_id("unique-id") == cmd | |||
|
|
|||
|
|
|||
| def test_command_state_preserves_handle_order( # noqa:D103 | |||
There was a problem hiding this comment.
I'm omitting docstrings and #noqa:D103ing these test functions because the function names are what show up in the PyTest summary, so it seems like I should invest all my descriptive energy into making those clear and readable.
If we think this is weird and inconsistent, I'm happy to add docstrings now and reserve this discussion for another day.
There was a problem hiding this comment.
There was a problem hiding this comment.
Yeah, I like that too.
When I try that, I feel like I frequently collide with the unfortunate combination of:
- The first docstring sentence must fit on a single line.
- A single line must be less than 80 (I think?) characters.
Like, I want the "it should..." sentence to be an elaboration of whatever the function name test_command_state_preserves_handle_order means...but there's not enough room for that, so it becomes a restatement instead.
Maybe I need to bite the verbosity bullet and do a cascade like:
def test_command_state_preserves_handle_order():
"""It should preserve handle order. <-- [This bit bugs me.]
It should return commands in the order they were initially added.
Replacing the command at an existing ID should keep the original
command's place in the ordering.
"""There was a problem hiding this comment.
With that I think the first sentence of your description is better, and this would be enough for me:
def test_command_state_preserves_order() -> None:
"""It should return commands in the order they are added."""
# ...There was a problem hiding this comment.
Moving forward with docstrings similar to that latest suggestion. Though it still feels a little unsatisfying to me, since it's not "semantically different" enough from the function name, if that makes sense.
Overview
Document, test, and implement
opentrons.protocol_engine.state.commands.get_next_request(), which was formerly aNotImplementedError, in service towards #7808.Review requests
Mostly on testing.
Risk assessment
None. Not production code, and this wasn't implemented before.