fix constant hostname in e2e tests #712
Conversation
WalkthroughThis PR removes hard-coded REST API service hostname and port specifications from e2e test feature files and introduces environment variable-based configuration in the test infrastructure code. Multiple feature files are updated to rely on defaults or external configuration instead of explicit localhost:8080 declarations. Changes
Sequence Diagram(s)sequenceDiagram
participant Test as Feature Test
participant Common as common.py
participant Env as environment.py
participant EnvVar as Environment Variables
Note over Common: Before: Hard-coded localhost:8080
Note over Common: After: Environment-based config
Test->>Common: Start scenario
Common->>EnvVar: Read E2E_LSC_HOSTNAME (or default localhost)
EnvVar-->>Common: hostname value
Common->>EnvVar: Read E2E_LSC_PORT (or default 8080)
EnvVar-->>Common: port value
Common->>Common: context.hostname, context.port initialized
Test->>Env: _fetch_models_from_service()
Env->>EnvVar: Read E2E_LSC_HOSTNAME, E2E_LSC_PORT
EnvVar-->>Env: hostname, port
Env->>Env: Build URL dynamically
Env-->>Test: Return models dict
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Rationale: While 10 feature files contain repetitive, low-complexity deletions (removing identical lines), the logic changes in environment.py and common.py require separate reasoning—specifically the signature change to Possibly related PRs
Suggested reviewers
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: 1
🧹 Nitpick comments (1)
tests/e2e/features/environment.py (1)
25-48: Update docstring and consider centralizing environment variable access.The docstring doesn't mention that the function now reads configuration from environment variables. Additionally, the environment variables
E2E_LSC_HOSTNAMEandE2E_LSC_PORTare read both here and intests/e2e/features/steps/common.py, creating duplication.Apply this diff to update the docstring:
-def _fetch_models_from_service() -> dict: - """Query /v1/models endpoint and return first LLM model. +def _fetch_models_from_service() -> dict: + """Query /v1/models endpoint and return first LLM model. + + Reads service endpoint from E2E_LSC_HOSTNAME and E2E_LSC_PORT environment variables.Consider refactoring to use
context.hostnameandcontext.portif they're already set, or centralizing the environment variable reading logic in a single location to avoid duplication and potential inconsistencies.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
tests/e2e/features/authorized_noop.feature(0 hunks)tests/e2e/features/authorized_noop_token.feature(0 hunks)tests/e2e/features/conversations.feature(0 hunks)tests/e2e/features/environment.py(4 hunks)tests/e2e/features/feedback.feature(0 hunks)tests/e2e/features/health.feature(0 hunks)tests/e2e/features/info.feature(0 hunks)tests/e2e/features/query.feature(0 hunks)tests/e2e/features/rest_api.feature(0 hunks)tests/e2e/features/smoketests.feature(0 hunks)tests/e2e/features/steps/common.py(1 hunks)tests/e2e/features/streaming_query.feature(0 hunks)
💤 Files with no reviewable changes (10)
- tests/e2e/features/conversations.feature
- tests/e2e/features/feedback.feature
- tests/e2e/features/query.feature
- tests/e2e/features/authorized_noop.feature
- tests/e2e/features/info.feature
- tests/e2e/features/rest_api.feature
- tests/e2e/features/authorized_noop_token.feature
- tests/e2e/features/health.feature
- tests/e2e/features/streaming_query.feature
- tests/e2e/features/smoketests.feature
🧰 Additional context used
📓 Path-based instructions (3)
**/*.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/environment.pytests/e2e/features/steps/common.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/environment.pytests/e2e/features/steps/common.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/common.py
🔇 Additional comments (2)
tests/e2e/features/environment.py (2)
158-161: Verify context attributes are set before use.Similar to the issue at line 111, this code assumes
context.hostnameandcontext.portare set by the "Given The service is started locally" step. Ensure all feature files that use the Feedback tag include this prerequisite step.The verification script from the previous comment will help confirm this.
102-111: No verification issues found—the code is safe.The original concern assumes context attributes may not be set when
after_scenarioruns, but this isn't a risk in Behave. In Behave, Background steps execute before every scenario, including those marked@skip. Since all 10 feature files include "Given The service is started locally" in their Background section (verified in the script output),context.hostname_llamaandcontext.port_llamaare guaranteed to be populated before any scenario executes. The code at lines 102–111 correctly uses these attributes, and the defensivehasattr()check on line 90 forllama_stack_was_runningis appropriate.
| @given("The service is started locally") | ||
| def service_is_started_locally(context: Context) -> None: | ||
| """Check the service status.""" | ||
| assert context is not None | ||
| context.hostname = os.getenv("E2E_LSC_HOSTNAME", "localhost") | ||
| context.port = os.getenv("E2E_LSC_PORT", "8080") | ||
| context.hostname_llama = os.getenv("E2E_LLAMA_HOSTNAME", "localhost") | ||
| context.port_llama = os.getenv("E2E_LLAMA_PORT", "8321") |
There was a problem hiding this comment.
🧩 Analysis chain
Update the docstring and verify port type expectations.
The docstring states "Check the service status" but the function actually configures the context with hostname and port values from environment variables. Additionally, ports are stored as strings, but downstream code may expect integers.
Apply this diff to update the docstring:
@given("The service is started locally")
def service_is_started_locally(context: Context) -> None:
- """Check the service status."""
+ """Configure context with service hostnames and ports from environment variables."""
assert context is not NoneRun the following script to verify whether downstream code expects ports as integers:
🏁 Script executed:
#!/bin/bash
# Description: Check if port values are used in numeric contexts or string interpolation
# Search for usage of context.port and context.port_llama
rg -n -C3 'context\.port' tests/e2e/Length of output: 12969
Update docstring and convert port strings to integers for type consistency.
The docstring "Check the service status" is misleading—the function actually configures context with hostname and port values from environment variables. Additionally, os.getenv() returns strings, but other code (e.g., set_service_port in common_http.py) sets context.port as an integer, creating a type inconsistency.
Apply this diff:
@given("The service is started locally")
def service_is_started_locally(context: Context) -> None:
- """Check the service status."""
+ """Configure context with service hostnames and ports from environment variables."""
assert context is not None
context.hostname = os.getenv("E2E_LSC_HOSTNAME", "localhost")
- context.port = os.getenv("E2E_LSC_PORT", "8080")
+ context.port = int(os.getenv("E2E_LSC_PORT", "8080"))
context.hostname_llama = os.getenv("E2E_LLAMA_HOSTNAME", "localhost")
- context.port_llama = os.getenv("E2E_LLAMA_PORT", "8321")
+ context.port_llama = int(os.getenv("E2E_LLAMA_PORT", "8321"))📝 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.
| @given("The service is started locally") | |
| def service_is_started_locally(context: Context) -> None: | |
| """Check the service status.""" | |
| assert context is not None | |
| context.hostname = os.getenv("E2E_LSC_HOSTNAME", "localhost") | |
| context.port = os.getenv("E2E_LSC_PORT", "8080") | |
| context.hostname_llama = os.getenv("E2E_LLAMA_HOSTNAME", "localhost") | |
| context.port_llama = os.getenv("E2E_LLAMA_PORT", "8321") | |
| @given("The service is started locally") | |
| def service_is_started_locally(context: Context) -> None: | |
| """Configure context with service hostnames and ports from environment variables.""" | |
| assert context is not None | |
| context.hostname = os.getenv("E2E_LSC_HOSTNAME", "localhost") | |
| context.port = int(os.getenv("E2E_LSC_PORT", "8080")) | |
| context.hostname_llama = os.getenv("E2E_LLAMA_HOSTNAME", "localhost") | |
| context.port_llama = int(os.getenv("E2E_LLAMA_PORT", "8321")) |
🤖 Prompt for AI Agents
In tests/e2e/features/steps/common.py around lines 8 to 15, the docstring is
misleading and port values are left as strings; update the docstring to describe
that the function configures context hostnames and ports from environment
variables, and convert E2E_LSC_PORT and E2E_LLAMA_PORT to integers (with safe
defaults) before assigning to context.port and context.port_llama so their types
match other code that expects ints.
2 participants
Description
Type of change
Related Tickets & Documents
Checklist before requesting a review
Testing
Summary by CodeRabbit
Tests