ci(release): handle giskard-llm#2430
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces a mechanism to filter tests by provider using the TEST_PROVIDER environment variable. Feedback includes adding the bare mark to the openai provider group for consistency with documentation and refactoring the provider parsing logic to avoid redundant environment variable lookups.
| # - ``openai`` includes ``bare`` (same OpenAI SDK, no ``configure()`` path). | ||
| # - ``google`` includes ``gemini`` (same ``google.genai`` SDK, different routing prefix). | ||
| _TEST_PROVIDER_GROUPS: dict[str, frozenset[str]] = { | ||
| "openai": frozenset({"openai"}), |
There was a problem hiding this comment.
The openai group is missing the bare mark, which contradicts the comment on line 20 stating that openai includes bare.
| "openai": frozenset({"openai"}), | |
| "openai": frozenset({"openai", "bare"}), |
| def _parse_test_provider() -> frozenset[str] | None: | ||
| """Return allowed provider marks, or ``None`` if filtering is disabled.""" | ||
|
|
||
| raw = os.environ.get("TEST_PROVIDER", "").strip() | ||
| if not raw or raw.lower() == "all": | ||
| return None | ||
| key = raw.lower() | ||
| return _TEST_PROVIDER_GROUPS.get(key, frozenset({key})) |
There was a problem hiding this comment.
The environment variable TEST_PROVIDER is fetched and stripped again in pytest_collection_modifyitems. It would be cleaner to have _parse_test_provider return both the allowed set and the raw display name to avoid redundant lookups.
def _parse_test_provider() -> tuple[frozenset[str], str] | None:\n """Return (allowed provider marks, display name), or ``None`` if filtering is disabled."""\n\n raw = os.environ.get("TEST_PROVIDER", "").strip()\n if not raw or raw.lower() == "all":\n return None\n key = raw.lower()\n return _TEST_PROVIDER_GROUPS.get(key, frozenset({key})), raw| allowed = _parse_test_provider() | ||
| if allowed is not None: | ||
| tp_disp = os.environ.get("TEST_PROVIDER", "").strip() or "?" | ||
| for item in items: |
There was a problem hiding this comment.
This change utilizes the updated _parse_test_provider to avoid redundant environment variable lookups.
parsed = _parse_test_provider()\n if parsed is not None:\n allowed, tp_disp = parsed\n for item in items:…alongside allowed marks
No description provided.