Could you move this arg below the constants. In this codebase, the conventional structure is class constants, then variables, then functions

You do not need to move this check to process_report.py. We only want to move the fetching of the env var. Error checking can stay in the invoice class for now.

You should add CHROME_BIN_PATH in REQUIRED_ENV_VARS

There's no reason to perform validation in two different places. If we're using pydantic_settings to manage environment variables, then rather than using required_env_files here, just mark chrome_bin_path as required in process_report.settings.Settings. Currently it is optional:

    chrome_bin_path: str | None = None

To make it required:

    chrome_bin_path: str

This will cause pydantic to throw a validation error when instantiating Settings:

pydantic_core._pydantic_core.ValidationError: 1 validation error for Settings
chrome_bin_path
  Field required [type=missing, input_value={}, input_type=dict]
    For further information visit https://errors.pydantic.dev/2.10/v/missing

Similarly, you can remove the check for KEYCLOAK_CLIENT_ID and KEYCLOAK_CLIENT_SECRET here by adding a validator to Settings:

from pydantic import model_validator
from pydantic_settings import BaseSettings

class Settings(BaseSettings):
    ...

    
    @model_validator(mode="after")
    def check_keycloak_auth(self):
        if not self.coldfront_api_filepath and not (
            self.keycloak_client_id and self.keycloak_client_secret
        ):
            raise ValueError(
                "You must either set coldfront_api_filepath or provide keycloak credentials in "
                "KEYCLOAK_CLIENT_ID and KEYCLOAK_CLIENT_SECRET"
            )

You probably want to catch the exception from pydantic to produce a more friendly error message that does not include a full Python traceback.

Thank you for the very detailed feedback. I don't know why I didn't thought of getting rid of that hacky function in the first place. Your feedback makes it look obvious

Related to the adding CHROME_BIN_PATH to REQUIRED_ENV_VARS, you should have the env var fetched after the env vars have been validated

What was the motivation for this change? I think it's generally better practice to read your environment variables early, rather than doing it inline like this.

I've written so that the env var is loaded by the settings module

Why are we setting CHROME_BIN_PATH to an invalid path here?

The unit tests were designed to not require integration with 3rd-party tools. We do have an actual integration test for Chrome that installs Chromium and sets a real path.

As for why the env var is set, rather than not set at all, I wanted to check that code actual read the env var CHROME_BIN_PATH, whatever it maybe.

I don't really like this. Is there any way you can provide these values through mocking during the tests?

To echo what Kristi said: unit tests should never depend on the value of external environment variables. Regardless of whether CHROME_BIN_PATH is set to an invalid value, a valid path, or is unset, the unit tests should behave the same.

You don't even need to use mocking; you can just set environment variables directly in your unit tests

There's no reason to perform validation in two different places. If we're using pydantic_settings to manage environment variables, then rather than using required_env_files here, just mark chrome_bin_path as required in process_report.settings.Settings. Currently it is optional:

    chrome_bin_path: str | None = None

To make it required:

    chrome_bin_path: str

This will cause pydantic to throw a validation error when instantiating Settings:

pydantic_core._pydantic_core.ValidationError: 1 validation error for Settings
chrome_bin_path
  Field required [type=missing, input_value={}, input_type=dict]
    For further information visit https://errors.pydantic.dev/2.10/v/missing

Similarly, you can remove the check for KEYCLOAK_CLIENT_ID and KEYCLOAK_CLIENT_SECRET here by adding a validator to Settings:

from pydantic import model_validator
from pydantic_settings import BaseSettings

class Settings(BaseSettings):
    ...

    
    @model_validator(mode="after")
    def check_keycloak_auth(self):
        if not self.coldfront_api_filepath and not (
            self.keycloak_client_id and self.keycloak_client_secret
        ):
            raise ValueError(
                "You must either set coldfront_api_filepath or provide keycloak credentials in "
                "KEYCLOAK_CLIENT_ID and KEYCLOAK_CLIENT_SECRET"
            )

You probably want to catch the exception from pydantic to produce a more friendly error message that does not include a full Python traceback.

I think it makes sense to keep providing a default as before as you've now made the environment variable required for running all unit tests for no obvious gain.

I don't really like this. Is there any way you can provide these values through mocking during the tests?