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(api): Port AddressableAreaStore to StateUpdate #17027

Merged
merged 17 commits into from
Dec 10, 2024

Conversation

SyntaxColoring
Copy link
Contributor

@SyntaxColoring SyntaxColoring commented Dec 3, 2024

Overview

Closes EXEC-755.

Test Plan and Hands on Testing

  • Make sure all automated tests (in particular, the analysis snapshot tests) keep passing.
  • Run some random protocols on a Flex and OT-2 and make sure there aren't any unexpected errors related to addressable areas or deck configurations.

Changelog

Following the usual pattern of EXEC-639, port:

  • AddressableAreaStore
  • LoadLabwareImplementation
  • MoveToAddressableAreaImplementation
  • MoveToAddressableAreaForDropTipImplementation
  • LoadModuleImplementation

So that the command implementations use StateUpdate to tell AddressableAreaStore what addressable area they have used, instead of AddressableAreaStore trying to figure it out for itself.

Review requests

See the risk assessment below and the comments on the diff.

Risk assessment

Medium.

This needs careful code review to make sure that the end result on AddressableAreaState is unchanged; the code removed from AddressableAreaStore should be exactly compensated by the code added to the command implementations.

These command implementations did not have full unit test coverage of all the stuff they did with addressable areas and deck configurations, and even if they did, it's very possible for me to mess things up. I initially had one bug where I had LoadModuleImplementation load a completely wrong addressable area, and it was only caught by an analysis snapshot test that I guess happened to have a specific deck layout.

LoadLabwareImplementation and its tests in particular have grown to be difficult to verify.

@SyntaxColoring SyntaxColoring requested review from a team December 4, 2024 17:16
@SyntaxColoring SyntaxColoring marked this pull request as ready for review December 4, 2024 19:05
@SyntaxColoring SyntaxColoring requested a review from a team as a code owner December 4, 2024 19:05
Copy link
Contributor

@jbleon95 jbleon95 left a comment

Choose a reason for hiding this comment

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

This all looks accurate to me, I answered the questions I could. Thanks for updating this.

Copy link
Contributor

@CaseyBatten CaseyBatten left a comment

Choose a reason for hiding this comment

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

This all looks good, thanks for the migration. Test coverage looks good, particularly on the module addressable areas. Commented on the hanging question regarding module load location.

@SyntaxColoring
Copy link
Contributor Author

Thank you both!

@SyntaxColoring SyntaxColoring merged commit 19d3483 into edge Dec 10, 2024
21 checks passed
@SyntaxColoring SyntaxColoring deleted the aastore_to_stateupdate branch December 10, 2024 16:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants