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

chore(api): port absorbance reader commands to state update #17113

Merged
merged 31 commits into from
Jan 2, 2025

Conversation

TamarZanzouri
Copy link
Contributor

@TamarZanzouri TamarZanzouri commented Dec 16, 2024

Overview

absorbance reader part of module state update.
https://opentrons.atlassian.net/browse/EXEC-756.
adding missing command unit tests for absorbance reader.

Test Plan and Hands on Testing

upload a protocol using the absorbance reader.
make sure all commands are working as expected.
make sure state is updated as expected.

Changelog

  • added state update methods for absorbance reader state.
  • added unit test files for absorbance reader commands.

Review requests

  1. state update changes make sense? what do you think about having a class for all module state updates?
  2. did I miss anything in the tests (I did not implement the commands)? do they make sense?

Risk assessment

low (if I did this properly). module state updates and missing tests.

@TamarZanzouri TamarZanzouri marked this pull request as ready for review December 23, 2024 21:54
@TamarZanzouri TamarZanzouri requested a review from a team as a code owner December 23, 2024 21:54
Copy link

codecov bot commented Dec 24, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 92.43%. Comparing base (906d841) to head (716ec08).
Report is 83 commits behind head on edge.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             edge   #17113   +/-   ##
=======================================
  Coverage   92.43%   92.43%           
=======================================
  Files          77       77           
  Lines        1283     1283           
=======================================
  Hits         1186     1186           
  Misses         97       97           
Flag Coverage Δ
g-code-testing 92.43% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

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.

Looks really good and the porting over to state update passes immediate sight checks for the same kind of behavior we had implemented already. Test coverage also looks good, don't see anything missing there. Thanks for doing this Tamar!

Comment on lines +194 to +195
state_update.files_added = update_types.FilesAddedUpdate(file_ids=file_ids)

Copy link
Contributor

Choose a reason for hiding this comment

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

This logic looks solid down through it's usage. We utilize the files added data at the end of a run on the client as when looking through the run result to populate the list of files available for download via the app after the fact, so this should maintain that logic.

Copy link
Contributor

@SyntaxColoring SyntaxColoring left a comment

Choose a reason for hiding this comment

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

Looks great, thank you!

Thank you for filling in the missing tests.

We're still missing tests for ModuleStateStore, but that was the case before, and it certainly doesn't need to get fixed in this PR. It does mean that we have to be careful about reviewing the _handle_absorbance_reader_commands() code by eye, though.

Copy link
Contributor

@SyntaxColoring SyntaxColoring left a comment

Choose a reason for hiding this comment

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

TY!

@TamarZanzouri TamarZanzouri merged commit 576501a into edge Jan 2, 2025
24 checks passed
@TamarZanzouri TamarZanzouri deleted the EXEC-756-port-module-store-updates branch January 2, 2025 18:48
@TamarZanzouri TamarZanzouri restored the EXEC-756-port-module-store-updates branch January 2, 2025 18:49
@TamarZanzouri TamarZanzouri deleted the EXEC-756-port-module-store-updates branch January 2, 2025 18:49
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