LCORE-760: fix hardcoded model and provider in e2e tests #680
Conversation
WalkthroughThe PR parameterizes E2E test model and provider values by introducing dynamic model discovery and placeholder substitution. A new utility function replaces Changes
Sequence DiagramsequenceDiagram
participant Test as E2E Test Runner
participant Env as environment.py<br/>(before_all)
participant Service as /v1/models<br/>Endpoint
participant Context as Behave Context
participant Step as Step Implementation
participant Utils as replace_placeholders()
participant API as API Endpoint
Test->>Env: Run before_all()
Env->>Service: _fetch_models_from_service()
Service-->>Env: [{model_id, provider_id, ...}]
Env->>Context: Set default_model, default_provider
Env->>Test: Setup complete
Test->>Step: Execute feature step<br/>(payload with {MODEL}, {PROVIDER})
Step->>Utils: replace_placeholders(context, payload)
Utils->>Context: Read default_model, default_provider
Utils-->>Step: Substituted JSON payload
Step->>API: Send request with actual values
API-->>Step: Response
Step->>Test: Assertion passed
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes The changes follow a consistent, repetitive pattern of placeholder substitution across multiple files, reducing cognitive load per-file. However, the scope spans environment setup, utility functions, feature files, and multiple step implementations, requiring verification that model discovery integrates correctly and placeholders substitute uniformly across all test paths. Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
tests/e2e/features/environment.py (2)
30-34: Consider parameterizing the timeout.The timeout is hardcoded to 5 seconds. Consider making it configurable via environment variable or function parameter to support different network conditions.
56-58: Use logging module instead of print statements.Per coding guidelines, use
logger = logging.getLogger(__name__)for module logging. Print statements don't integrate with test frameworks' logging infrastructure and may not be visible in CI environments.As per coding guidelines, add at the top of the file:
import logging logger = logging.getLogger(__name__)Then replace the print statements:
- print( - f"Detected LLM: {context.default_model} (provider: {context.default_provider})" - ) + logger.info( + f"Detected LLM: {context.default_model} (provider: {context.default_provider})" + )And for the warning:
- print("⚠ Could not detect models, using fallback: gpt-4-turbo/openai") + logger.warning("Could not detect models, using fallback: gpt-4-turbo/openai")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
tests/e2e/features/conversations.feature(1 hunks)tests/e2e/features/environment.py(1 hunks)tests/e2e/features/info.feature(1 hunks)tests/e2e/features/query.feature(3 hunks)tests/e2e/features/steps/common_http.py(2 hunks)tests/e2e/features/steps/conversation.py(2 hunks)tests/e2e/features/steps/info.py(1 hunks)tests/e2e/features/steps/llm_query_response.py(4 hunks)tests/e2e/features/streaming_query.feature(3 hunks)tests/e2e/utils/utils.py(2 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
tests/e2e/features/**/*.feature
📄 CodeRabbit inference engine (CLAUDE.md)
Write E2E tests as Gherkin feature files for behave
Files:
tests/e2e/features/query.featuretests/e2e/features/info.featuretests/e2e/features/conversations.featuretests/e2e/features/streaming_query.feature
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: All modules start with descriptive module-level docstrings explaining purpose
Use logger = logging.getLogger(name) for module logging after import logging
Define type aliases at module level for clarity
All functions require docstrings with brief descriptions
Provide complete type annotations for all function parameters and return types
Use typing_extensions.Self in model validators where appropriate
Use modern union syntax (str | int) and Optional[T] or T | None consistently
Function names use snake_case with descriptive, action-oriented prefixes (get_, validate_, check_)
Avoid in-place parameter modification; return new data structures instead of mutating arguments
Use appropriate logging levels: debug, info, warning, error with clear messages
All classes require descriptive docstrings explaining purpose
Class names use PascalCase with conventional suffixes (Configuration, Error/Exception, Resolver, Interface)
Abstract base classes should use abc.ABC and @AbstractMethod for interfaces
Provide complete type annotations for all class attributes
Follow Google Python docstring style for modules, classes, and functions, including Args, Returns, Raises, Attributes sections as needed
Files:
tests/e2e/features/steps/conversation.pytests/e2e/features/environment.pytests/e2e/features/steps/llm_query_response.pytests/e2e/features/steps/info.pytests/e2e/features/steps/common_http.pytests/e2e/utils/utils.py
tests/e2e/features/steps/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Place behave step definitions under tests/e2e/features/steps/
Files:
tests/e2e/features/steps/conversation.pytests/e2e/features/steps/llm_query_response.pytests/e2e/features/steps/info.pytests/e2e/features/steps/common_http.py
tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
tests/**/*.py: Use pytest-mock to create AsyncMock objects for async interactions in tests
Use the shared auth mock constant: MOCK_AUTH = ("mock_user_id", "mock_username", False, "mock_token") in tests
Files:
tests/e2e/features/steps/conversation.pytests/e2e/features/environment.pytests/e2e/features/steps/llm_query_response.pytests/e2e/features/steps/info.pytests/e2e/features/steps/common_http.pytests/e2e/utils/utils.py
🧬 Code graph analysis (3)
tests/e2e/features/steps/conversation.py (1)
tests/e2e/utils/utils.py (1)
replace_placeholders(147-160)
tests/e2e/features/steps/llm_query_response.py (1)
tests/e2e/utils/utils.py (1)
replace_placeholders(147-160)
tests/e2e/features/steps/common_http.py (1)
tests/e2e/utils/utils.py (1)
replace_placeholders(147-160)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: build-pr
- GitHub Check: e2e_tests (ci)
- GitHub Check: e2e_tests (azure)
🔇 Additional comments (14)
tests/e2e/features/environment.py (1)
50-63: Verify fallback values match typical deployment configuration.The fallback to "gpt-4-turbo" and "openai" ensures tests continue when model discovery fails. Ensure these fallback values match your typical deployment configuration to avoid confusing test failures when the service is misconfigured.
Do the fallback values match the models typically available in your test environment?
tests/e2e/features/info.feature (1)
37-37: LGTM!Changing from a specific model assertion to a generic "proper model structure" check appropriately supports the parameterized test approach where model/provider values are discovered dynamically.
tests/e2e/features/conversations.feature (1)
25-25: LGTM!The placeholder substitution for model and provider values correctly aligns with the dynamic test parameterization approach used across the test suite.
tests/e2e/features/steps/conversation.py (1)
113-116: Ensure context attributes are always initialized.The placeholder replacement relies on
context.default_modelandcontext.default_providerbeing set. Ifbefore_allinenvironment.pyfails to set these (e.g., both service query and fallback somehow fail), this will raiseAttributeError.Consider adding defensive checks or ensuring
before_allcannot skip setting these attributes.tests/e2e/features/streaming_query.feature (1)
54-54: LGTM!The placeholder substitution for model and provider values is consistent across all test scenarios and aligns with the dynamic test parameterization approach.
Also applies to: 67-67, 72-72, 79-79, 88-88
tests/e2e/features/query.feature (1)
45-45: LGTM!The placeholder values are correctly substituted throughout the query test scenarios, maintaining consistency with the other feature files in the PR.
Also applies to: 79-79, 84-84, 92-92, 102-102
tests/e2e/features/steps/common_http.py (1)
174-177: Placeholder replacement is safe.before_allalways initializescontext.default_modelandcontext.default_providerin both branches.tests/e2e/features/steps/llm_query_response.py (4)
7-7: LGTM!The import is correctly placed and the utility function is used consistently throughout the file.
28-32: LGTM!The placeholder replacement logic is correctly implemented with proper fallback handling for
Nonevalues.
43-47: LGTM!The implementation is consistent with the other step functions and correctly applies placeholder replacement.
66-73: LGTM!The placeholder replacement is correctly implemented and follows the same pattern as the other step functions.
tests/e2e/features/steps/info.py (3)
40-47: LGTM!The model selection logic correctly finds the first LLM model and includes proper null checking with a clear error message.
30-32: Confirm correct step usage The feature file (tests/e2e/features/info.feature:37) references the step without parameters, so removing themodelargument is safe.
49-65: No action needed: context.default_model and context.default_provider are initialized
Detection logic and fallback inenvironment.pyensure these attributes are set before use.
| def _fetch_models_from_service(hostname: str = "localhost", port: int = 8080) -> dict: | ||
| """Query /v1/models endpoint and return first LLM model. | ||
| Returns: | ||
| Dict with model_id and provider_id, or empty dict if unavailable | ||
| """ | ||
| try: | ||
| url = f"http://{hostname}:{port}/v1/models" | ||
| response = requests.get(url, timeout=5) | ||
| response.raise_for_status() | ||
| data = response.json() | ||
|
|
||
| # Find first LLM model | ||
| for model in data.get("models", []): | ||
| if model.get("api_model_type") == "llm": | ||
| provider_id = model.get("provider_id") | ||
| model_id = model.get("provider_resource_id") | ||
| if provider_id and model_id: | ||
| return {"model_id": model_id, "provider_id": provider_id} | ||
| return {} | ||
| except (requests.RequestException, ValueError, KeyError): | ||
| return {} |
There was a problem hiding this comment.
Remove dead exception handler.
The except block catches KeyError, but the code uses .get() method (lines 37-42) which returns None instead of raising KeyError. This makes the KeyError handler unreachable.
Apply this diff:
- except (requests.RequestException, ValueError, KeyError):
+ except (requests.RequestException, ValueError):
return {}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def _fetch_models_from_service(hostname: str = "localhost", port: int = 8080) -> dict: | |
| """Query /v1/models endpoint and return first LLM model. | |
| Returns: | |
| Dict with model_id and provider_id, or empty dict if unavailable | |
| """ | |
| try: | |
| url = f"http://{hostname}:{port}/v1/models" | |
| response = requests.get(url, timeout=5) | |
| response.raise_for_status() | |
| data = response.json() | |
| # Find first LLM model | |
| for model in data.get("models", []): | |
| if model.get("api_model_type") == "llm": | |
| provider_id = model.get("provider_id") | |
| model_id = model.get("provider_resource_id") | |
| if provider_id and model_id: | |
| return {"model_id": model_id, "provider_id": provider_id} | |
| return {} | |
| except (requests.RequestException, ValueError, KeyError): | |
| return {} | |
| def _fetch_models_from_service(hostname: str = "localhost", port: int = 8080) -> dict: | |
| """Query /v1/models endpoint and return first LLM model. | |
| Returns: | |
| Dict with model_id and provider_id, or empty dict if unavailable | |
| """ | |
| try: | |
| url = f"http://{hostname}:{port}/v1/models" | |
| response = requests.get(url, timeout=5) | |
| response.raise_for_status() | |
| data = response.json() | |
| # Find first LLM model | |
| for model in data.get("models", []): | |
| if model.get("api_model_type") == "llm": | |
| provider_id = model.get("provider_id") | |
| model_id = model.get("provider_resource_id") | |
| if provider_id and model_id: | |
| return {"model_id": model_id, "provider_id": provider_id} | |
| return {} | |
| except (requests.RequestException, ValueError): | |
| return {} |
🤖 Prompt for AI Agents
In tests/e2e/features/environment.py around lines 24 to 45, the except block
currently catches KeyError which cannot be raised because all dict accesses use
.get(); remove KeyError from the exception tuple so the handler only catches
actual possible errors (e.g., requests.RequestException and ValueError). Update
the except clause to "except (requests.RequestException, ValueError):" and leave
the body returning an empty dict.
| def replace_placeholders(context: Context, text: str) -> str: | ||
| """Replace {MODEL} and {PROVIDER} placeholders with actual values from context. | ||
| Args: | ||
| context: Behave context containing default_model and default_provider | ||
| text: String that may contain {MODEL} and {PROVIDER} placeholders | ||
| Returns: | ||
| String with placeholders replaced by actual values | ||
| """ | ||
| result = text.replace("{MODEL}", context.default_model) | ||
| result = result.replace("{PROVIDER}", context.default_provider) | ||
| return result |
There was a problem hiding this comment.
Add defensive attribute checks.
The function accesses context.default_model and context.default_provider without verifying they exist. If before_all in environment.py fails to set these attributes, this will raise AttributeError with an unclear error message.
Apply this diff to add defensive checks:
def replace_placeholders(context: Context, text: str) -> str:
"""Replace {MODEL} and {PROVIDER} placeholders with actual values from context.
Args:
context: Behave context containing default_model and default_provider
text: String that may contain {MODEL} and {PROVIDER} placeholders
Returns:
String with placeholders replaced by actual values
+
+ Raises:
+ AttributeError: If context is missing default_model or default_provider
"""
+ if not hasattr(context, 'default_model') or not hasattr(context, 'default_provider'):
+ raise AttributeError(
+ "Context missing required attributes: default_model and/or default_provider. "
+ "Ensure before_all in environment.py executed successfully."
+ )
+
result = text.replace("{MODEL}", context.default_model)
result = result.replace("{PROVIDER}", context.default_provider)
return result📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def replace_placeholders(context: Context, text: str) -> str: | |
| """Replace {MODEL} and {PROVIDER} placeholders with actual values from context. | |
| Args: | |
| context: Behave context containing default_model and default_provider | |
| text: String that may contain {MODEL} and {PROVIDER} placeholders | |
| Returns: | |
| String with placeholders replaced by actual values | |
| """ | |
| result = text.replace("{MODEL}", context.default_model) | |
| result = result.replace("{PROVIDER}", context.default_provider) | |
| return result | |
| def replace_placeholders(context: Context, text: str) -> str: | |
| """Replace {MODEL} and {PROVIDER} placeholders with actual values from context. | |
| Args: | |
| context: Behave context containing default_model and default_provider | |
| text: String that may contain {MODEL} and {PROVIDER} placeholders | |
| Returns: | |
| String with placeholders replaced by actual values | |
| Raises: | |
| AttributeError: If context is missing default_model or default_provider | |
| """ | |
| if not hasattr(context, 'default_model') or not hasattr(context, 'default_provider'): | |
| raise AttributeError( | |
| "Context missing required attributes: default_model and/or default_provider. " | |
| "Ensure before_all in environment.py executed successfully." | |
| ) | |
| result = text.replace("{MODEL}", context.default_model) | |
| result = result.replace("{PROVIDER}", context.default_provider) | |
| return result |
🤖 Prompt for AI Agents
In tests/e2e/utils/utils.py around lines 147 to 160, the function
replace_placeholders uses context.default_model and context.default_provider
directly which can raise AttributeError if before_all didn't set them; update
the function to defensively read these attributes via getattr(context,
"default_model", None) and getattr(context, "default_provider", None), then if
either is None raise a clear RuntimeError (or ValueError) with a descriptive
message about missing test setup and how to fix it (or optionally accept
sensible fallback defaults), and use those retrieved values when performing the
replacements so the error is explicit and not an AttributeError.
2 participants
Description
fix hardcoded model and provider in e2e tests
Type of change
Related Tickets & Documents
Checklist before requesting a review
Testing
Summary by CodeRabbit
Release Notes