Skip to content

Merge back 'chore_release-pd-8.5.0' into 'edge' #18820

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

Merged
merged 24 commits into from
Jul 3, 2025
Merged

Conversation

ddcc4
Copy link
Collaborator

@ddcc4 ddcc4 commented Jul 2, 2025

This picks up the changes from chore_release-8.5.0 and chore_release-pd-8.5.0 from the past few days.

Manually resolved conflicts in:

  • protocol-designer/src/pages/Designer/ProtocolSteps/StepForm/StepTools/MoveLiquidTools/hooks/useAssignLiquidClass.ts

ryanthecoder and others added 22 commits June 27, 2025 14:55
…t. (#18742)

<!--
Thanks for taking the time to open a Pull Request (PR)! Please make sure
you've read the "Opening Pull Requests" section of our Contributing
Guide:


https://github.com/Opentrons/opentrons/blob/edge/CONTRIBUTING.md#opening-pull-requests

GitHub provides robust markdown to format your PR. Links, diagrams,
pictures, and videos along with text formatting make it possible to
create a rich and informative PR. For more information on GitHub
markdown, see:


https://docs.github.com/en/get-started/writing-on-github/getting-started-with-writing-and-formatting-on-github/basic-writing-and-formatting-syntax

To ensure your code is reviewed quickly and thoroughly, please fill out
the sections below to the best of your ability!
-->

# Overview
Tuck in the last few hardware-testing changes into the 8.5 release.
<!--
Describe your PR at a high level. State acceptance criteria and how this
PR fits into other work. Link issues, PRs, and other relevant resources.
-->

## Test Plan and Hands on Testing
Does not touch public code so no testing required.
<!--
Describe your testing of the PR. Emphasize testing not reflected in the
code. Attach protocols, logs, screenshots and any other assets that
support your testing.
-->

## Changelog

<!--
List changes introduced by this PR considering future developers and the
end user. Give careful thought and clear documentation to breaking
changes.
-->

## Review requests

<!--
- What do you need from reviewers to feel confident this PR is ready to
merge?
- Ask questions.
-->

## Risk assessment

<!--
- Indicate the level of attention this PR needs.
- Provide context to guide reviewers.
- Discuss trade-offs, coupling, and side effects.
- Look for the possibility, even if you think it's small, that your
change may affect some other part of the system.
- For instance, changing return tip behavior may also change the
behavior of labware calibration.
- How do your unit tests and on hands on testing mitigate this PR's
risks and the risk of future regressions?
- Especially in high risk PRs, explain how you know your testing is
enough.
-->
# Overview

In PR #18711 (AUTH-2003), we changed the liquid class display names to:
`Aqueous (Deionized water)` / `Volatile (80% ethanol)` / `Viscous (50% glycerol)`.

But we've decided we want to change the display names back. So:
- displayName: `Aqueous`, description `Deionized water`, loadname: `water`
- displayName: `Volatile`, description `80% ethanol`, loadname: `ethanol_80`
- displayName: `Viscous`, description `50% glycerol`, loadname: `glycerol_50`

https://opentrons.slack.com/archives/C07JJA1AJFQ/p1751036532344239?thread_ts=1750692306.637929&cid=C07JJA1AJFQ

This PR reverts #18711 and #18738, and was generated by `git revert`
with no manual changes:
```
git revert ec54caf 968ddc8
```

## Test Plan and Hands on Testing

Running CI tests.

## Review requests

This is what we want now, right?

## Risk assessment

Low, but we're cutting it close.
Don't crash the entire app when this happens.

Closes RQA-4308
)

Correctly pass `dispenseRetractPositionReference` rather than
`dispensePositionReference` to `getIsRetractSafeForAirGap` in
`distribute` and `transfer` compound command creators.
<!--
Thanks for taking the time to open a Pull Request (PR)! Please make sure
you've read the "Opening Pull Requests" section of our Contributing
Guide:


https://github.com/Opentrons/opentrons/blob/edge/CONTRIBUTING.md#opening-pull-requests

GitHub provides robust markdown to format your PR. Links, diagrams,
pictures, and videos along with text formatting make it possible to
create a rich and informative PR. For more information on GitHub
markdown, see:


https://docs.github.com/en/get-started/writing-on-github/getting-started-with-writing-and-formatting-on-github/basic-writing-and-formatting-syntax

To ensure your code is reviewed quickly and thoroughly, please fill out
the sections below to the best of your ability!
-->

# Overview

Updates to the Liquid Control page for PD interop + new params added. 


<!--
Describe your PR at a high level. State acceptance criteria and how this
PR fits into other work. Link issues, PRs, and other relevant resources.
-->

## Test Plan and Hands on Testing

<!--
Describe your testing of the PR. Emphasize testing not reflected in the
code. Attach protocols, logs, screenshots and any other assets that
support your testing.
-->
sandbox: http://sandbox.docs.opentrons.com/docs-pd-interop-8.5/v2/

## Changelog

-added description for ``flow_rate`` param in aspirate/dispense commands
(and contrast with ``rate``). checked the API reference entries.
- added description for air gap ``in_place`` and ``flow_rate``
parameters (and contrast with ``rate``)
- added description for ``mm_from_edge`` behavior and warning. 
<!--
List changes introduced by this PR considering future developers and the
end user. Give careful thought and clear documentation to breaking
changes.
-->

## Review requests

<!--
- What do you need from reviewers to feel confident this PR is ready to
merge?
- Ask questions.
-->
Need to confirm the warning for ``mm_from_edge`` in touch tip. 

## Risk assessment

<!--
- Indicate the level of attention this PR needs.
- Provide context to guide reviewers.
- Discuss trade-offs, coupling, and side effects.
- Look for the possibility, even if you think it's small, that your
change may affect some other part of the system.
- For instance, changing return tip behavior may also change the
behavior of labware calibration.
- How do your unit tests and on hands on testing mitigate this PR's
risks and the risk of future regressions?
- Especially in high risk PRs, explain how you know your testing is
enough.
-->

low.
This reverts #18450 and #18479 to revert to digicert signing for our
windows builds.

The digicert certificate has the Common Name "Opentrons Labworks Inc."
and the ATS cert has the common name "OPENTRONS LABWORKS INC.". These
were both determined automatically by the CA from our identity
submissions, as is apparently required in the code signing cert baseline
requirements. Why are they different? A mystery for the ages.

In either case, electron-updater requires that _if_ you specify a
`publisherName` in your `app-update.yml` (which we do specifically on
windows, since it is generated from our electron-builder config and on
windows we set it because nsis packager wants it for doing signing in
the first place) _then_ the autoupdate package that will be installed
must have a CSC CN exactly matching an entry in `publisherName` or the
update will fail. Therefore updates in between <=8.4.1 and >=8.5.0 would
fail if we switched to ATS.

Instead, we'll switch back to digicert for now; we'll build the new CN
into our publisher names; and then whenever we're confident enough
people are on >=8.5.0 and therefore have the new publisher names, we'll
switch over again (we can't switch immediately because we don't do
incremental updates, just full overwrites, so the intermediate update
state would go away).

This is upsetting.

## Testing
- [x] the signing has to work again, which is never guaranteed given the
shonky state of dco integration
- [x] we should make sure we can update from 8.4.1 to this by making
sure the CN of the digital signature on the build from this pr is
exactly `Opentrons Labworks Inc.` (and updating to it in the resulting
alpha)
- [x] we should make sure we can update from this to something signed
with the new cert (by mucking around with the latest-alpha or something?
or just checking that the app-update.yml in the app's install directory
has both names)

Supercedes #18785 for build branch name reasons.
…#18802)

# Overview

In `distribute_with_liquid_class()`, we were dispensing into the same
destination well twice when the distribute consists of multiple
tip-fills.

The bug was caused by the reuse of the variable `next_dest` in
`distribute_with_liquid_class()`. `next_dest` in the outer loop is
supposed to contain the next well that we have not dispensed to yet.
However, we accidently overwrote the variable because we also use it as
a loop variable for an unrelated inner loop:
```
for next_vol, next_dest in vol_dest_combo:
```

## Test Plan and Hands on Testing

Using `analyze`, I confirmed that the double-dispense happened with
Andy's example before this change, and that it was fixed after this
change.

I also updated
`test_order_of_water_distribution_steps_using_multi_dispense()` to
perform a distribute that requires multiple tip-fills, and changed the
test to check that we're dispensing into specific wells instead of
`dest=mock.ANY`. I confirmed that the bug happened before this change,
and it was fixed after this change.

## Risk assessment

Medium: this is change deep inside our implementation. But it's
necessary to fix the bug.
…e well (#18791)

This PR fixes the subtext in the liquid class options in a transfer or
mix form. We should only show "Assigned to [x,y,z wells]" subtext to a
liquid class option if the liquid class is assigned to a liquid in at
least one of the selected source/mix wells. If the liquid class is
present in the source/mix labwrae, but not present in the selected
source wells, we show the liquid class's generic description.

Closes RQA-4106
This PR ensures the specified dispense delay for a move liquid form is
applied after dispensing air gap.
…18808)

This PR fixes a whitescreen that occurs after deleting a pipette that is
used in a moveLiquid or mix step form.
# Overview

Addresses #18803 

> The documentation found
[here](https://docs.opentrons.com/v2/parameters/defining.html?#the-add-parameters-function),
i.e. the "The add_parameters() Function" section of the page on
"Defining Parameters", has the wrong type annotation. It is documented
as Parameters when it should be ParameterContext.

## Test Plan and Hands on Testing


[Sandbox](http://sandbox.docs.opentrons.com/docs-fix-rtp-annotation/v2/parameters/defining.html#the-add-parameters-function)

## Changelog

1-liner

## Review requests

nothing special.

## Risk assessment

nil
Ensures OT-2 liquid class default values for aspirate/dispense
submerge/retract z height are populated.

Closes RQA-4345
…sfers (#18818)

Calculate correction by volume by keying on the volume after aspiration and dispense, not the aspirate or dispense volume itself.
Ensure `pushOut` param for air gap `dispenseInPlace` commands is
explicitly 0.
…18816)

Complete the behavior for getting OT-2 default values when resetting
advanced transfer settings or creating a new transfer form.

Closes RQA-4344
Manually resolved conflicts:
- app-shell/electron-builder.config.js
- components/src/organisms/CommandText/__tests__/CommandText.test.tsx
- protocol-designer/src/pages/Designer/ProtocolSteps/StepForm/StepTools/MoveLiquidTools/__tests__/LiquidClassesStepTools.test.tsx
- react-api-client/src/system/useAuthorization.ts
@ddcc4 ddcc4 requested review from koji, ncdiehl11 and jerader July 2, 2025 22:54
@ddcc4 ddcc4 requested review from a team as code owners July 2, 2025 22:54
Copy link

codecov bot commented Jul 2, 2025

Codecov Report

Attention: Patch coverage is 30.60498% with 195 lines in your changes missing coverage. Please review.

Project coverage is 27.07%. Comparing base (3ad5220) to head (4d1c163).
Report is 124 commits behind head on edge.

Files with missing lines Patch % Lines
...r/src/steplist/formLevel/handleFormChange/utils.ts 0.00% 67 Missing ⚠️
app-shell/scripts/windows-custom-sign.js 0.00% 66 Missing ⚠️
...ools/MoveLiquidTools/hooks/useAssignLiquidClass.ts 0.00% 36 Missing ⚠️
...ring/utils/commandText/getMoveToWellCommandText.ts 52.63% 9 Missing ⚠️
app-shell/electron-builder.config.js 0.00% 5 Missing ⚠️
...tocolSteps/StepForm/PipetteFields/TiprackField.tsx 0.00% 4 Missing ⚠️
protocol-designer/src/step-forms/reducers/index.ts 0.00% 3 Missing ⚠️
...eration/src/commandCreators/compound/distribute.ts 83.33% 2 Missing ⚠️
...ration/src/commandCreators/compound/consolidate.ts 50.00% 1 Missing ⚠️
...eneration/src/commandCreators/compound/transfer.ts 92.30% 1 Missing ⚠️
... and 1 more
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             edge   #18820      +/-   ##
==========================================
- Coverage   27.70%   27.07%   -0.63%     
==========================================
  Files        3204     3246      +42     
  Lines      250439   257281    +6842     
  Branches    26320    26866     +546     
==========================================
+ Hits        69380    69671     +291     
- Misses     181042   187588    +6546     
- Partials       17       22       +5     
Flag Coverage Δ
app 1.24% <0.00%> (-0.73%) ⬇️
protocol-designer 19.10% <17.26%> (+<0.01%) ⬆️
step-generation 5.35% <14.38%> (+0.08%) ⬆️

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

Files with missing lines Coverage Δ
.../organisms/ConfirmDeleteEntityInUseModal/index.tsx 100.00% <100.00%> (ø)
...components/organisms/LiquidsOverflowMenu/index.tsx 94.62% <100.00%> (ø)
.../components/organisms/SelectLabwareModal/index.tsx 59.10% <100.00%> (-0.08%) ⬇️
...rc/components/organisms/TipPositionModal/index.tsx 78.91% <100.00%> (ø)
...file/migration/utils/getLoadLiquidClassCommands.ts 92.15% <100.00%> (ø)
...t/formLevel/stepFormToArgs/moveLiquidFormToArgs.ts 81.14% <100.00%> (ø)
...igner/src/steplist/formLevel/test/warnings.test.ts 98.26% <100.00%> (ø)
...tocol-designer/src/steplist/formLevel/warnings.tsx 85.61% <100.00%> (ø)
step-generation/src/utils/index.ts 100.00% <100.00%> (ø)
step-generation/src/utils/liquidClassUtils.ts 99.52% <100.00%> (ø)
... and 12 more

... and 283 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@SyntaxColoring
Copy link
Contributor

Hm that failing test is a little concerning. A protocol is coming out the other end of a PD migration without any commands?

Or is that expected from the export-to-Python work?

@ddcc4
Copy link
Collaborator Author

ddcc4 commented Jul 3, 2025

that failing test is a little concerning. A protocol is coming out the other end of a PD migration without any commands?
Or is that expected from the export-to-Python work?

I ran it again, and it passed -- must be a flaky test :( https://github.com/Opentrons/opentrons/actions/runs/16037463114/job/45256158103#step:4:175

I think Josh explained that the Cypress tests sometimes fail because they're clicking too fast before the PD app has gotten to the step they were expecting. So in this case, the mis-timed clicks probably failed to create a protocol with commands to export.

@jerader
Copy link
Collaborator

jerader commented Jul 3, 2025

that failing test is a little concerning. A protocol is coming out the other end of a PD migration without any commands?
Or is that expected from the export-to-Python work?

I ran it again, and it passed -- must be a flaky test :( https://github.com/Opentrons/opentrons/actions/runs/16037463114/job/45256158103#step:4:175

I think Josh explained that the Cypress tests sometimes fail because they're clicking too fast before the PD app has gotten to the step they were expecting. So in this case, the mis-timed clicks probably failed to create a protocol with commands to export.

Yeah, the PD cypress migration test is very flakey unfortunately.

Copy link
Collaborator

@jerader jerader left a comment

Choose a reason for hiding this comment

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

lgtm

Resolved conflicts: `protocol-designer/src/pages/Designer/ProtocolSteps/StepForm/StepTools/MoveLiquidTools/hooks/useAssignLiquidClass.ts`.
@ddcc4 ddcc4 force-pushed the merge-pd-8.5.0-into-edge branch from 8dea32a to 4d1c163 Compare July 3, 2025 15:20
@ddcc4 ddcc4 merged commit 4d1c163 into edge Jul 3, 2025
90 of 93 checks passed
@ddcc4 ddcc4 deleted the merge-pd-8.5.0-into-edge branch July 3, 2025 15:58
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.

10 participants