Skip to content

Commit a00cb11

Browse files
fix(hardware-testing): fix hanging gravi analysis (#18685)
You can't really use click.test's isolated environments because it needs to be able to import hardware testing. It also needs to clean up its recording thread on failure in addition to on success, and finally doing these thigns reveals some failures we'll also have to fix. --------- Co-authored-by: Ryan Howard <[email protected]>
1 parent 22e2651 commit a00cb11

File tree

5 files changed

+50
-23
lines changed

5 files changed

+50
-23
lines changed

hardware-testing/.flake8

Lines changed: 2 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1,16 +1,13 @@
11
[flake8]
22

3-
# set line-length for future black support
4-
# https://github.com/psf/black/blob/master/docs/compatible_configs.md
5-
max-line-length = 100
6-
73
# max cyclomatic complexity
84
# NOTE: (andy s) increasing this from 9 to 15 b/c test scripts often handle all logic in main
95
max-complexity = 15
106

117
extend-ignore =
12-
# ignore E203 because black might reformat it
8+
# black handles formatting
139
E203,
10+
E501,
1411
# do not require type annotations for self nor cls
1512
ANN101,
1613
ANN102

hardware-testing/hardware_testing/gravimetric/measurement/record.py

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,5 @@
11
"""Record weight measurements."""
2+
23
from contextlib import contextmanager
34
from dataclasses import dataclass
45
from statistics import stdev

hardware-testing/hardware_testing/gravimetric/protocol_replacement/96ch1000.csv

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,7 @@ return_tip,TRUE,,,,,,,,
88
touch_tip,FALSE,,,,,,,,
99
liquid,water,Deionized water,#0000ff,100000,,,,,
1010
tipracks_20ul,,,,,,,,,
11-
tipracks_50ul,D2
11+
tipracks_50ul,D2,D3,C2,C3,B1,B2
1212
tipracks_200ul,,,,,,,,,
1313
tipracks_1000ul,,,,,,,,,
1414
labware_on_scale,nest_1_reservoir_195ml,A1,,,,,,,

hardware-testing/hardware_testing/gravimetric/protocol_replacement/gravimetric.py

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -650,6 +650,14 @@ def run_one_test(
650650
def run(ctx: ProtocolContext) -> None:
651651
"""Run."""
652652
fixture_settings = FixtureSettings.build(ctx)
653+
try:
654+
_do_run(ctx, fixture_settings)
655+
finally:
656+
if fixture_settings.recorder.is_recording:
657+
fixture_settings.recorder.stop()
658+
659+
660+
def _do_run(ctx: ProtocolContext, fixture_settings: FixtureSettings) -> None:
653661
first_tip = fixture_settings.tips[list(fixture_settings.tips)[0]].pop(0)
654662
print_info("Picking up first tip.")
655663
pick_up_tip_for_channel(fixture_settings, first_tip, 1)

hardware-testing/tests/hardware_testing/gravimetric/test_gravimetric_protocol.py

Lines changed: 38 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -3,12 +3,13 @@
33
import json
44
import tempfile
55

6+
import subprocess
67
from dataclasses import dataclass
78
from typing import Any, Dict, List, Optional
89
from pathlib import Path
910

10-
from click.testing import CliRunner
11-
from opentrons.cli.analyze import analyze
11+
import pytest
12+
1213
from opentrons.protocols.api_support.definitions import MAX_SUPPORTED_VERSION
1314

1415

@@ -23,7 +24,7 @@
2324
class _AnalysisCLIResult:
2425
exit_code: int
2526
json_output: Optional[Dict[str, Any]]
26-
stdout_stderr: str
27+
stdout_stderr: bytes
2728

2829

2930
# Function copied from api/tests/opentrons/cli/test_cli.py
@@ -45,8 +46,14 @@ def _get_analysis_result(
4546
"""
4647
with tempfile.TemporaryDirectory() as temp_dir:
4748
analysis_output_file = Path(temp_dir) / "analysis_output.json"
48-
runner = CliRunner()
49-
args = [output_type, str(analysis_output_file)]
49+
args = [
50+
"python",
51+
"-m",
52+
"opentrons.cli",
53+
"analyze",
54+
output_type,
55+
str(analysis_output_file),
56+
]
5057

5158
if rtp_values is not None:
5259
args.extend(["--rtp-values", rtp_values])
@@ -59,32 +66,46 @@ def _get_analysis_result(
5966
if check:
6067
args.append("--check")
6168

62-
result = runner.invoke(analyze, args)
69+
process = subprocess.run(args, stdout=subprocess.PIPE, stderr=subprocess.STDOUT)
6370
if analysis_output_file.exists():
6471
json_output = json.loads(analysis_output_file.read_bytes())
6572
else:
6673
json_output = None
6774
return _AnalysisCLIResult(
68-
exit_code=result.exit_code,
75+
exit_code=process.returncode,
6976
json_output=json_output,
70-
stdout_stderr=result.output,
77+
stdout_stderr=process.stdout,
7178
)
7279

7380

74-
# TODO (spp, 2025-06-10): update this test to verify that the gravimetric protocol
75-
# analyzes successfully once the protocol and CSV file values are corrected.
76-
# It's better to write a *single* test that verifies all aspects of its analysis result
77-
# than writing a different one for each aspect since the gravimetric test is complex
78-
# and analyzing it takes a long time.
79-
def test_gravimetric_test_protocol_uses_latest_api_version() -> None:
80-
"""Should check that gravimetric test protocol uses the latest Python API version."""
81+
@pytest.mark.parametrize(
82+
"pipette",
83+
[
84+
pytest.param(
85+
"96ch200", marks=pytest.mark.xfail(reason="200ul has no liquid class")
86+
),
87+
pytest.param("96ch1000"),
88+
],
89+
)
90+
def test_gravimetric_test_protocol_passes_analysis(pipette: str) -> None:
91+
"""Check that gravimetric test protocol uses the latest Python API version and simulates."""
8192
result = _get_analysis_result(
8293
[GRAVIMETRIC_PROTOCOL_FILEPATH],
8394
"--json-output",
84-
rtp_files=json.dumps({"qc_test_profile": str(CSV_FILEPATH.resolve())}),
95+
rtp_files=json.dumps(
96+
{
97+
"qc_test_profile": str(
98+
(GRAVIMETRIC_PROTOCOL_PARENT_FILEPATH / f"{pipette}.csv").resolve()
99+
)
100+
}
101+
),
85102
)
103+
print(result.stdout_stderr)
86104
assert result.exit_code == 0
87-
assert result.json_output is not None
105+
assert result.json_output
106+
assert result.json_output["errors"] == [], "Analysis failed: " + str(
107+
result.json_output
108+
)
88109
assert result.json_output["config"]["apiVersion"] == [
89110
MAX_SUPPORTED_VERSION.major,
90111
MAX_SUPPORTED_VERSION.minor,

0 commit comments

Comments
 (0)