Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor(robot-server): split apart experimental sessions router #8057

Merged
merged 3 commits into from
Jul 7, 2021

Conversation

mcous
Copy link
Contributor

@mcous mcous commented Jul 6, 2021

Overview

This PR is precursor work to #7921, split into its own PR to ease reviews. It is a pure reorganization PR; it does not affect functionality.

Changelog

This PR splits the previously monolithic robot_server/sessions/router.py into three separate routers:

  • robot_server/sessions/router/base_router.py for /sessions routes
  • robot_server/sessions/router/commands_router.py for /sessions/.../commands routes
  • robot_server/sessions/router/actions_router.py for /sessions/.../actions routes

The same corresponding change is made to the tests, with required changes to keep things green.

Review requests

  • Code was moved without changes
  • Test changes make sense
    • In the case of the /commands router, I think the test changes make it clear that this split is beneficial

Risk assessment

Very low. This entire router is feature flagged, and test coverage of the router was and remains high

@codecov
Copy link

codecov bot commented Jul 6, 2021

Codecov Report

Merging #8057 (7315937) into edge (4d8529a) will increase coverage by 7.37%.
The diff coverage is 97.33%.

Impacted file tree graph

@@            Coverage Diff             @@
##             edge    #8057      +/-   ##
==========================================
+ Coverage   85.88%   93.25%   +7.37%     
==========================================
  Files         356      135     -221     
  Lines       21520     5263   -16257     
==========================================
- Hits        18482     4908   -13574     
+ Misses       3038      355    -2683     
Impacted Files Coverage Δ
...ver/robot_server/sessions/router/actions_router.py 93.10% <93.10%> (ø)
...ot-server/robot_server/sessions/router/__init__.py 100.00% <100.00%> (ø)
...server/robot_server/sessions/router/base_router.py 100.00% <100.00%> (ø)
...er/robot_server/sessions/router/commands_router.py 100.00% <100.00%> (ø)
opentrons/protocols/models/__init__.py
opentrons/protocol_api/contexts.py
opentrons/drivers/types.py
opentrons/motion_planning/errors.py
opentrons/file_runner/__init__.py
opentrons/protocol_api_experimental/errors.py
... and 221 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4d8529a...7315937. Read the comment docs.

Copy link
Contributor Author

@mcous mcous left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some previously wrong test docstrings that we should fix up

robot-server/tests/sessions/router/test_actions_router.py Outdated Show resolved Hide resolved
robot-server/tests/sessions/router/test_actions_router.py Outdated Show resolved Hide resolved
Base automatically changed from engine_python-runner to edge July 6, 2021 22:34
@mcous mcous force-pushed the robot-server_sessions-router-reorg branch from 2fd8771 to 1fd5d0b Compare July 6, 2021 22:36
@mcous mcous added robot-svcs Falls under the purview of the Robot Services squad (formerly CPX, Core Platform Experience). protocol-engine Ticket related to the Protocol Engine project and associated HTTP APIs robot server Affects the `robot-server` project labels Jul 6, 2021
@mcous mcous marked this pull request as ready for review July 6, 2021 22:37
@mcous mcous requested a review from a team as a code owner July 6, 2021 22:37
Copy link
Member

@sanni-t sanni-t left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

)


@pytest.fixture(autouse=True)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice!

spec=real_get_session,
)
# TODO(mc, 2021-07-06): add signature support in decoy
get_session.__signature__ = inspect.signature( # type: ignore[attr-defined]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this required because decoy.create_decoy_func(spec=real_get_session) doesn't copy the type signatures of the spec?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, filed a ticket for it mcous/decoy#29. Should be pretty straghtforward; decoy spies need to implement the __signature__ magic property

Co-authored-by: Sanniti Pimpley <[email protected]>
@mcous mcous merged commit 5549266 into edge Jul 7, 2021
@mcous mcous deleted the robot-server_sessions-router-reorg branch July 7, 2021 17:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
protocol-engine Ticket related to the Protocol Engine project and associated HTTP APIs robot server Affects the `robot-server` project robot-svcs Falls under the purview of the Robot Services squad (formerly CPX, Core Platform Experience).
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants