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

Config setup docs - through tests #634

Merged
merged 12 commits into from
Oct 30, 2024
Merged

Config setup docs - through tests #634

merged 12 commits into from
Oct 30, 2024

Conversation

stan-dot
Copy link
Contributor

please validate the directional correctness of the test first. #tdd

@stan-dot stan-dot linked an issue Sep 12, 2024 that may be closed by this pull request
@stan-dot stan-dot self-assigned this Sep 12, 2024
@stan-dot stan-dot changed the title inital tests outline Adding test for config setup Sep 12, 2024
@stan-dot stan-dot added the tests Issues around increasing test coverage/fixing tests label Sep 12, 2024
@stan-dot stan-dot force-pushed the 532-docs-on-config-setup branch from 93899f7 to 4d6fbb3 Compare September 13, 2024 08:57
@stan-dot stan-dot changed the title Adding test for config setup Config setup docs - through tests Sep 30, 2024
@stan-dot stan-dot force-pushed the 532-docs-on-config-setup branch from 4d6fbb3 to 4db0aa4 Compare September 30, 2024 14:51
@stan-dot
Copy link
Contributor Author

stan-dot commented Oct 7, 2024

it works until line 179 ( call to loader.use_values_from_yaml)

@stan-dot
Copy link
Contributor Author

stan-dot commented Oct 7, 2024

    config_data = {
        "env": {},
        "sources": [
            {"kind": "dodal", "module": "dodal.adsim"},
            {"kind": "planFunctions", "module": "dls_bluesky_core.plans"},
            {"kind": "planFunctions", "module": "dls_bluesky_core.stubs"},
        ],
        "data_writing": {
            "visit_directory": "/dls/p38/data/2023/cm33874-1",
            "group_name": "BL38P",
        },
    }

does not map directly to this:

#532 (comment)

perhaps config loading could be simplified, to mirror more closely the helm structure

@stan-dot stan-dot marked this pull request as ready for review October 7, 2024 10:33
@stan-dot stan-dot requested a review from tpoliaw October 7, 2024 10:33
@stan-dot
Copy link
Contributor Author

stan-dot commented Oct 7, 2024

all good

Copy link

codecov bot commented Oct 7, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 92.46%. Comparing base (d3a8d9f) to head (326d1b1).
Report is 1 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #634   +/-   ##
=======================================
  Coverage   92.45%   92.46%           
=======================================
  Files          35       35           
  Lines        1657     1658    +1     
=======================================
+ Hits         1532     1533    +1     
  Misses        125      125           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

tests/unit_tests/test_config.py Outdated Show resolved Hide resolved
tests/unit_tests/test_config.py Show resolved Hide resolved
tests/unit_tests/test_config.py Outdated Show resolved Hide resolved
tests/unit_tests/test_config.py Show resolved Hide resolved
tests/unit_tests/test_config.py Outdated Show resolved Hide resolved
@stan-dot
Copy link
Contributor Author

class BasicAuthentication(BaseModel):
    """
    User credentials for basic authentication
    """

    username: str = Field(description="Unique identifier for user")
    password: Secret[str] = Field(description="Password to verify user's identity")

    @field_validator("username", "password", mode="before")
    @classmethod
    def get_from_env(cls, v: str):
        if v.startswith("${") and v.endswith("}"):
            return os.environ[v.removeprefix("${").removesuffix("}").upper()]
        return v

current error is coming from bluesky-stomp, where for some reason it uses the class method to get the password from env. therefore full validation from a file might require an adjustment

@stan-dot
Copy link
Contributor Author

@ZohebShaikh any ideas?

@DiamondJoseph
Copy link
Contributor

DiamondJoseph commented Oct 14, 2024

class BasicAuthentication(BaseModel):
    """
    User credentials for basic authentication
    """

    username: str = Field(description="Unique identifier for user")
    password: Secret[str] = Field(description="Password to verify user's identity")

    @field_validator("username", "password", mode="before")
    @classmethod
    def get_from_env(cls, v: str):
        if v.startswith("${") and v.endswith("}"):
            return os.environ[v.removeprefix("${").removesuffix("}").upper()]
        return v

current error is coming from bluesky-stomp, where for some reason it uses the class method to get the password from env. therefore full validation from a file might require an adjustment

AttributeError: 'Secret' object has no attribute 'startswith'

@stan-dot you need to unwrap the Secret[str] before using it.

It fetches it from the ENV if configured to do so for injection of the RMQ password. If you want to adjust how the handling of that env var fetching is done to your override levels handling then feel free. @keithralphs needs to mount a bunch of tracing values as env vars, so having that handling in one place is preferable. But the type of the password should remain Secret[str] to prevent accidental logging.

@stan-dot stan-dot force-pushed the 532-docs-on-config-setup branch from f567ae7 to 59226b9 Compare October 16, 2024 13:48
@stan-dot
Copy link
Contributor Author

File "/opt/hostedtoolcache/Python/3.10.15/x64/lib/python3.10/site-packages/bluesky_stomp/models.py", line 17, in get_from_env
if v.startswith("${") and v.endswith("}"):
AttributeError: 'Secret' object has no attribute 'startswith'

odd, digging deeper

@stan-dot
Copy link
Contributor Author

FAILED tests/unit_tests/test_config.py::test_config_yaml_parsed_complete[temp_yaml_config_file0] - blueapi.utils.invalid_config_error.InvalidConfigError: Something is wrong with the configuration file: 
 {'type': 'model_type', 'loc': ('scratch', 'repositories', 0), 'msg': 'Input should be a valid dictionary or instance of ScratchRepository', 'input': 'dodal', 'ctx': {'class_name': 'ScratchRepository'}, 'url': 'https://errors.pydantic.dev/2.9/v/model_type'}
FAILED tests/unit_tests/test_config.py::test_config_yaml_parsed_complete[temp_yaml_config_file1] - blueapi.utils.invalid_config_error.InvalidConfigError: Something is wrong with the configuration file: 
 {'type': 'model_type', 'loc': ('scratch', 'repositories', 0), 'msg': 'Input should be a valid dictionary or instance of ScratchRepository', 'input': 'dodal', 'ctx': {'class_name': 'ScratchRepository'}, 'url': 'https://errors.pydantic.dev/2.9/v/model_type'}
FAILED tests/unit_tests/worker/test_task_worker.py::test_worker_and_data_events_produce_in_order - concurrent.futures._base.TimeoutError
============= 3 failed, 

got a more specific error

@stan-dot stan-dot force-pushed the 532-docs-on-config-setup branch from 2483a41 to 4c6ae93 Compare October 28, 2024 11:40
Comment on lines 245 to 253
# Parameterized test to run with different configurations
# todo uncomment in python3.12, this here is for reference
# type config_type = (
# dict[str, str | int | dict[str, str]]
# | dict[str, dict[str, bool] | list[dict[str, str]]]
# | dict[str, str | int]
# | dict[str, str]
# | dict[str, str | list[dict[str, str]]]
# )
Copy link
Contributor

Choose a reason for hiding this comment

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

Should: Uncomment and use or remove

@stan-dot stan-dot force-pushed the 532-docs-on-config-setup branch from fc9eec9 to 9c8fb85 Compare October 29, 2024 12:33
Copy link
Contributor

@callumforrester callumforrester left a comment

Choose a reason for hiding this comment

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

LGTM, thanks

@stan-dot stan-dot force-pushed the 532-docs-on-config-setup branch from 9c8fb85 to 326d1b1 Compare October 30, 2024 14:04
@stan-dot stan-dot merged commit 452eed1 into main Oct 30, 2024
27 checks passed
@stan-dot stan-dot deleted the 532-docs-on-config-setup branch October 30, 2024 16:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tests Issues around increasing test coverage/fixing tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Docs on config setup
3 participants