Skip to content

Commit

Permalink
Fix settings tests (#14810)
Browse files Browse the repository at this point in the history
# Overview

When we patch the _read_settings_file function, we are calling to
fixtures that exclude flags that have internal_only set to True. When
you look at the _read_settings_file function there is no filtering out
of flags set to internal_only.

```python
def _read_settings_file(settings_file: "Path") -> SettingsData:
    """
    Read the settings file, which is a json object with settings IDs as keys
    and boolean values. For each key, look up the `Settings` object with that
    key. If the key is one of the old IDs (kebab case), replace it with the
    new ID and rewrite the settings file

    :param settings_file: the path to the settings file
    :return: a dict with all new settings IDs as the keys, and boolean values
        (the values stored in the settings file, or `False` if the key was not
        found). Along with the version.
    """
    # Read settings from persistent file
    data = _read_json_file(settings_file)
    settings, version = _migrate(data)
    settings = _ensure(settings)

    if data.get("_version") != version:
        _write_settings_file(settings, version, settings_file)

    return SettingsData(settings_map=settings, version=version)
```

Therefore, the patches for the OT-2 and Flex should be calling the
fixtures that do not exclude the internal_only flag.
  • Loading branch information
DerekMaggio committed Apr 8, 2024
1 parent c1c8929 commit e415a4a
Show file tree
Hide file tree
Showing 3 changed files with 19 additions and 11 deletions.
3 changes: 2 additions & 1 deletion api/src/opentrons/config/advanced_settings.py
Original file line number Diff line number Diff line change
Expand Up @@ -250,7 +250,7 @@ class Setting(NamedTuple):
),
robot_type=[RobotTypeEnum.OT2, RobotTypeEnum.FLEX],
internal_only=True,
)
),
]

if (
Expand Down Expand Up @@ -719,6 +719,7 @@ def _migrate31to32(previous: SettingsMap) -> SettingsMap:
newmap["enableOEMMode"] = None
return newmap


def _migrate32to33(previous: SettingsMap) -> SettingsMap:
"""Migrate to version 33 of the feature flags file.
Expand Down
4 changes: 1 addition & 3 deletions api/src/opentrons/config/feature_flags.py
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,4 @@ def enable_error_recovery_experiments() -> bool:


def enable_performance_metrics(robot_type: RobotTypeEnum) -> bool:
return advs.get_setting_with_env_overload(
"enablePerformanceMetrics", robot_type
)
return advs.get_setting_with_env_overload("enablePerformanceMetrics", robot_type)
23 changes: 16 additions & 7 deletions api/tests/opentrons/config/test_advanced_settings.py
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,15 @@ def mock_settings_values_flex() -> Dict[str, Optional[bool]]:
}


@pytest.fixture
def mock_settings_values_flex_all() -> Dict[str, Optional[bool]]:
return {
s.id: False
for s in advanced_settings.settings
if RobotTypeEnum.FLEX in s.robot_type
}


@pytest.fixture
def mock_settings_values_empty() -> Dict[str, Optional[bool]]:
return {s.id: None for s in advanced_settings.settings}
Expand All @@ -57,25 +66,25 @@ def mock_settings(

@pytest.fixture
def mock_read_settings_file_ot2(
mock_settings_values_ot2: Dict[str, Optional[bool]],
mock_settings_values_ot2_all: Dict[str, Optional[bool]],
mock_settings_version: int,
) -> Generator[MagicMock, None, None]:
with patch("opentrons.config.advanced_settings._read_settings_file") as p:
p.return_value = advanced_settings.SettingsData(
settings_map=mock_settings_values_ot2,
settings_map=mock_settings_values_ot2_all,
version=mock_settings_version,
)
yield p


@pytest.fixture
def mock_read_settings_file_flex(
mock_settings_values_flex: Dict[str, Optional[bool]],
mock_settings_values_flex_all: Dict[str, Optional[bool]],
mock_settings_version: int,
) -> Generator[MagicMock, None, None]:
with patch("opentrons.config.advanced_settings._read_settings_file") as p:
p.return_value = advanced_settings.SettingsData(
settings_map=mock_settings_values_flex,
settings_map=mock_settings_values_flex_all,
version=mock_settings_version,
)
yield p
Expand Down Expand Up @@ -168,19 +177,19 @@ def test_get_all_adv_settings_empty(

async def test_set_adv_setting(
mock_read_settings_file_ot2: MagicMock,
mock_settings_values_ot2: MagicMock,
mock_settings_values_ot2_all: MagicMock,
mock_write_settings_file: MagicMock,
mock_settings_version: int,
restore_restart_required: None,
) -> None:
for k, v in mock_settings_values_ot2.items():
for k, v in mock_settings_values_ot2_all.items():
# Toggle the advanced setting
await advanced_settings.set_adv_setting(k, not v)
mock_write_settings_file.assert_called_with(
# Only the current key is toggled
{
nk: nv if nk != k else not v
for nk, nv in mock_settings_values_ot2.items()
for nk, nv in mock_settings_values_ot2_all.items()
},
mock_settings_version,
CONFIG["feature_flags_file"],
Expand Down

0 comments on commit e415a4a

Please sign in to comment.