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(api): raise an error if protocol's defined parameters have duplicate variable names #14888

Merged
merged 1 commit into from
Apr 12, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions api/src/opentrons/protocol_api/_parameter_context.py
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,7 @@ def add_int(
description: A description of the parameter as it will show up on the frontend.
unit: An optional unit to be appended to the end of the integer as it shown on the frontend.
"""
validation.validate_variable_name_unique(variable_name, set(self._parameters))
parameter = parameter_definition.create_int_parameter(
display_name=display_name,
variable_name=variable_name,
Expand Down Expand Up @@ -88,6 +89,7 @@ def add_float(
description: A description of the parameter as it will show up on the frontend.
unit: An optional unit to be appended to the end of the float as it shown on the frontend.
"""
validation.validate_variable_name_unique(variable_name, set(self._parameters))
parameter = parameter_definition.create_float_parameter(
display_name=display_name,
variable_name=variable_name,
Expand Down Expand Up @@ -115,6 +117,7 @@ def add_bool(
default: The default value the boolean parameter will be set to. This will be used in initial analysis.
description: A description of the parameter as it will show up on the frontend.
"""
validation.validate_variable_name_unique(variable_name, set(self._parameters))
parameter = parameter_definition.create_bool_parameter(
display_name=display_name,
variable_name=variable_name,
Expand Down Expand Up @@ -145,6 +148,7 @@ def add_str(
Mutually exclusive with minimum and maximum.
description: A description of the parameter as it will show up on the frontend.
"""
validation.validate_variable_name_unique(variable_name, set(self._parameters))
parameter = parameter_definition.create_str_parameter(
display_name=display_name,
variable_name=variable_name,
Expand Down
13 changes: 12 additions & 1 deletion api/src/opentrons/protocols/parameters/validation.py
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import keyword
from typing import List, Optional, Union, Literal
from typing import List, Set, Optional, Union, Literal

from .types import (
AllowedTypes,
Expand All @@ -16,6 +16,17 @@
DESCRIPTION_MAX_LEN = 100


def validate_variable_name_unique(
variable_name: str, other_variable_names: Set[str]
) -> None:
"""Validate that the given variable name is unique."""
if variable_name in other_variable_names:
raise ParameterNameError(
f'"{variable_name}" is already defined as a variable name for another parameter.'
f" All variable names must be unique."
)


def ensure_display_name(display_name: str) -> str:
"""Validate display name is within the character limit."""
if len(display_name) > DISPLAY_NAME_MAX_LEN:
Expand Down
31 changes: 28 additions & 3 deletions api/tests/opentrons/protocol_api/test_parameter_context.py
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,9 @@ def subject(api_version: APIVersion) -> ParameterContext:

def test_add_int(decoy: Decoy, subject: ParameterContext) -> None:
"""It should create and add an int parameter definition."""
subject._parameters["other_param"] = decoy.mock(
cls=mock_parameter_definition.ParameterDefinition
)
param_def = decoy.mock(cls=mock_parameter_definition.ParameterDefinition)
decoy.when(param_def.variable_name).then_return("my cool variable")
decoy.when(
Expand All @@ -60,6 +63,7 @@ def test_add_int(decoy: Decoy, subject: ParameterContext) -> None:
unit="foot candles",
)
).then_return(param_def)

subject.add_int(
display_name="abc",
variable_name="xyz",
Expand All @@ -70,11 +74,16 @@ def test_add_int(decoy: Decoy, subject: ParameterContext) -> None:
description="blah blah blah",
unit="foot candles",
)

assert param_def is subject._parameters["my cool variable"]
decoy.verify(mock_validation.validate_variable_name_unique("xyz", {"other_param"}))


def test_add_float(decoy: Decoy, subject: ParameterContext) -> None:
"""It should create and add a float parameter definition."""
subject._parameters["other_param"] = decoy.mock(
cls=mock_parameter_definition.ParameterDefinition
)
param_def = decoy.mock(cls=mock_parameter_definition.ParameterDefinition)
decoy.when(param_def.variable_name).then_return("my cooler variable")
decoy.when(mock_validation.ensure_float_value(12.3)).then_return(3.21)
Expand All @@ -83,7 +92,6 @@ def test_add_float(decoy: Decoy, subject: ParameterContext) -> None:
decoy.when(
mock_validation.ensure_float_choices([{"display_name": "foo", "value": 4.2}])
).then_return([{"display_name": "bar", "value": 2.4}])

decoy.when(
mock_parameter_definition.create_float_parameter(
display_name="abc",
Expand All @@ -96,6 +104,7 @@ def test_add_float(decoy: Decoy, subject: ParameterContext) -> None:
unit="lux",
)
).then_return(param_def)

subject.add_float(
display_name="abc",
variable_name="xyz",
Expand All @@ -106,11 +115,16 @@ def test_add_float(decoy: Decoy, subject: ParameterContext) -> None:
description="blah blah blah",
unit="lux",
)

assert param_def is subject._parameters["my cooler variable"]
decoy.verify(mock_validation.validate_variable_name_unique("xyz", {"other_param"}))


def test_add_bool(decoy: Decoy, subject: ParameterContext) -> None:
"""It should create and add a boolean parameter definition."""
subject._parameters["other_param"] = decoy.mock(
cls=mock_parameter_definition.ParameterDefinition
)
param_def = decoy.mock(cls=mock_parameter_definition.ParameterDefinition)
decoy.when(param_def.variable_name).then_return("my coolest variable")
decoy.when(
Expand All @@ -125,17 +139,23 @@ def test_add_bool(decoy: Decoy, subject: ParameterContext) -> None:
description="lorem ipsum",
)
).then_return(param_def)

subject.add_bool(
display_name="cba",
variable_name="zxy",
default=False,
description="lorem ipsum",
)

assert param_def is subject._parameters["my coolest variable"]
decoy.verify(mock_validation.validate_variable_name_unique("zxy", {"other_param"}))


def test_add_string(decoy: Decoy, subject: ParameterContext) -> None:
"""It should create and add a string parameter definition."""
subject._parameters["other_param"] = decoy.mock(
cls=mock_parameter_definition.ParameterDefinition
)
param_def = decoy.mock(cls=mock_parameter_definition.ParameterDefinition)
decoy.when(param_def.variable_name).then_return("my slightly less cool variable")
decoy.when(
Expand All @@ -147,14 +167,19 @@ def test_add_string(decoy: Decoy, subject: ParameterContext) -> None:
description="fee foo fum",
)
).then_return(param_def)

subject.add_str(
display_name="jkl",
variable_name="qwerty",
default="asdf",
choices=[{"display_name": "bar", "value": "aaa"}],
description="fee foo fum",
)

assert param_def is subject._parameters["my slightly less cool variable"]
decoy.verify(
mock_validation.validate_variable_name_unique("qwerty", {"other_param"})
)


def test_set_parameters(decoy: Decoy, subject: ParameterContext) -> None:
Expand Down Expand Up @@ -200,5 +225,5 @@ def test_export_parameters_for_protocol(
subject._parameters = {"foo": param_def_1, "bar": param_def_2}

result = subject.export_parameters_for_protocol()
assert result.x == "a" # type: ignore [attr-defined]
assert result.y == 1.23 # type: ignore [attr-defined]
assert result.x == "a" # type: ignore[attr-defined]
assert result.y == 1.23 # type: ignore[attr-defined]
7 changes: 7 additions & 0 deletions api/tests/opentrons/protocols/parameters/test_validation.py
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,13 @@
from opentrons.protocols.parameters import validation as subject


def test_validate_variable_name_unique() -> None:
"""It should no-op if the name is unique and raise if it is not."""
subject.validate_variable_name_unique("one of a kind", {"fee", "foo", "fum"})
with pytest.raises(ParameterNameError):
subject.validate_variable_name_unique("copy", {"paste", "copy", "cut"})


def test_ensure_display_name() -> None:
"""It should ensure the display name is within the character limit."""
result = subject.ensure_display_name("abc")
Expand Down
Loading