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

fix(test): snapshot renaming #15038

Merged
merged 14 commits into from
May 1, 2024
Merged

fix(test): snapshot renaming #15038

merged 14 commits into from
May 1, 2024

Conversation

y3rsh
Copy link
Collaborator

@y3rsh y3rsh commented Apr 29, 2024

Cleanup snapshots

  • Given the new protocol organization and naming
    • delete all the snapshots of the old naming find -type f ! \( -name '*_S_*' -o -name '*_X_*' \) -exec rm {} +
    • make build-opentrons-analysis (default on edge)
    • make snapshot-test-update
    • find old snapshots and delete

@y3rsh y3rsh self-assigned this Apr 29, 2024
@y3rsh y3rsh requested a review from a team as a code owner April 29, 2024 18:58
@y3rsh y3rsh marked this pull request as draft April 29, 2024 19:04
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

👀 on this

seems a big change

Copy link
Member

@sanni-t sanni-t Apr 30, 2024

Choose a reason for hiding this comment

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

This is an expected change following #15002. We now use the latest pipette version's flow rates as the default rates during protocol analysis/ simulation.
For this particular protocol, the P50 pipette's default flow rate has changed from 8 to 35 uL/sec, default dispense changed from 8 to 57 uL/sec, etc

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

👀 at this

Copy link
Contributor

Choose a reason for hiding this comment

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

@y3rsh You resolved this thread—this is expected? Out of curiosity, do you know why?

@y3rsh y3rsh marked this pull request as ready for review April 30, 2024 14:15
@y3rsh y3rsh requested a review from a team as a code owner April 30, 2024 14:15
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.

This all looks reasonable to me, though it's difficult to be exhaustive because of the assorted semantic changes that are happening concurrently with the renames.

This is reaffirming for me that we should run these snapshot tests on each PR, to spread the work of verifying them and keep the diffs tied to the underlying changes that caused them, but I'm curious if you have any alternative solutions.

@y3rsh
Copy link
Collaborator Author

y3rsh commented May 1, 2024

This all looks reasonable to me, though it's difficult to be exhaustive because of the assorted semantic changes that are happening concurrently with the renames.

This is reaffirming for me that we should run these snapshot tests on each PR, to spread the work of verifying them and keep the diffs tied to the underlying changes that caused them, but I'm curious if you have any alternative solutions.

Yes. With the better error handling and organization in place, part 3 or 4 of my current effort is to get these running on PR.

@y3rsh y3rsh merged commit 4eac5a2 into edge May 1, 2024
7 checks passed
@y3rsh y3rsh deleted the RDEVOPS-86-part1-snapshot-renames branch May 1, 2024 14:03
Carlos-fernandez pushed a commit that referenced this pull request May 20, 2024
# Cleanup snapshots 

- Given the new protocol organization and naming
- delete all the snapshots of the old naming `find -type f ! \( -name
'*_S_*' -o -name '*_X_*' \) -exec rm {} +`
  - `make build-opentrons-analysis`  (default on edge)
  - ` make snapshot-test-update`
  - find old snapshots and delete
Carlos-fernandez pushed a commit that referenced this pull request Jun 3, 2024
- Given the new protocol organization and naming
- delete all the snapshots of the old naming `find -type f ! \( -name
'*_S_*' -o -name '*_X_*' \) -exec rm {} +`
  - `make build-opentrons-analysis`  (default on edge)
  - ` make snapshot-test-update`
  - find old snapshots and delete
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.

None yet

3 participants