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 settings tests #14810

Conversation

DerekMaggio
Copy link
Contributor

@DerekMaggio DerekMaggio commented Apr 4, 2024

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.

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.

When we patch the _read_settings_file function we are not including flags that are set to internal_only=True.
@DerekMaggio DerekMaggio requested review from a team as code owners April 4, 2024 20:38
@DerekMaggio DerekMaggio changed the base branch from edge to EXEC-380-create-performance-metrics-project April 4, 2024 20:39
@DerekMaggio DerekMaggio changed the base branch from EXEC-380-create-performance-metrics-project to EXEC-372-hide-performance-metrics-project-behind-ff April 4, 2024 20:39
@DerekMaggio DerekMaggio requested review from SyntaxColoring and removed request for SyntaxColoring April 4, 2024 20:52
Copy link

codecov bot commented Apr 4, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

❗ No coverage uploaded for pull request base (EXEC-372-hide-performance-metrics-project-behind-ff@ca74032). Click here to learn what that means.

Additional details and impacted files

Impacted file tree graph

@@                                  Coverage Diff                                   @@
##             EXEC-372-hide-performance-metrics-project-behind-ff   #14810   +/-   ##
======================================================================================
  Coverage                                                       ?   67.24%           
======================================================================================
  Files                                                          ?     2495           
  Lines                                                          ?    71254           
  Branches                                                       ?     8937           
======================================================================================
  Hits                                                           ?    47918           
  Misses                                                         ?    21235           
  Partials                                                       ?     2101           
Flag Coverage Δ
g-code-testing 92.43% <ø> (?)

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

Files Coverage Δ
api/src/opentrons/config/advanced_settings.py 94.47% <ø> (ø)
api/src/opentrons/config/feature_flags.py 100.00% <ø> (ø)

Copy link
Member

@sfoster1 sfoster1 left a comment

Choose a reason for hiding this comment

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

ty, good catch!

@DerekMaggio DerekMaggio merged commit be0683b into EXEC-372-hide-performance-metrics-project-behind-ff Apr 4, 2024
22 checks passed
DerekMaggio added a commit that referenced this pull request Apr 8, 2024
# 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.
DerekMaggio added a commit that referenced this pull request Apr 8, 2024
# 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.
@DerekMaggio DerekMaggio deleted the fix-settings-tests branch April 12, 2024 14:59
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.

2 participants