LCORE-1221: fix before all hook not to fail on prow#1428
Conversation
WalkthroughThe Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
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
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/e2e/features/steps/proxy.py`:
- Around line 39-40: The is_prow_environment() check is treating any defined
RUNNING_PROW value (including empty string) as truthy and can incorrectly force
context.is_docker_mode False; update is_prow_environment() to return a strict
boolean by reading the env var (e.g., os.getenv("RUNNING_PROW") in utils and
compare to a specific expected value like "1" or "true") and change the proxy
step to rely on that strict boolean; additionally, ensure per-scenario state is
reset by clearing or reinitializing context.is_docker_mode in the Behave
before_scenario hook (or equivalent) so the Docker mode decision
(context.is_docker_mode) cannot leak between scenarios.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 483ffe07-a2ee-48fd-9653-871a6e1a45ca
📒 Files selected for processing (1)
tests/e2e/features/steps/proxy.py
📜 Review details
⏰ 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). (5)
- GitHub Check: build-pr
- GitHub Check: E2E: library mode / ci
- GitHub Check: E2E: server mode / ci
- GitHub Check: E2E Tests for Lightspeed Evaluation job
- GitHub Check: Konflux kflux-prd-rh02 / lightspeed-stack-on-pull-request
🧰 Additional context used
📓 Path-based instructions (3)
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Use absolute imports for internal modules from the Lightspeed Core Stack (e.g.,
from authentication import get_auth_dependency)
Files:
tests/e2e/features/steps/proxy.py
tests/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
tests/**/*.py: Use pytest for all unit and integration tests; do not use unittest
Maintain test coverage of at least 60% for unit tests and 10% for integration tests
Files:
tests/e2e/features/steps/proxy.py
tests/e2e/features/steps/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Define E2E test step implementations in
tests/e2e/features/steps/directory
Files:
tests/e2e/features/steps/proxy.py
🧠 Learnings (1)
📚 Learning: 2025-08-25T09:11:38.701Z
Learnt from: radofuchs
Repo: lightspeed-core/lightspeed-stack PR: 445
File: tests/e2e/features/environment.py:0-0
Timestamp: 2025-08-25T09:11:38.701Z
Learning: In the Behave Python testing framework, the Context object is created once for the entire test run and reused across all features and scenarios. Custom attributes added to the context persist until explicitly cleared, so per-scenario state should be reset in hooks to maintain test isolation.
Applied to files:
tests/e2e/features/steps/proxy.py
🔇 Additional comments (1)
tests/e2e/features/steps/proxy.py (1)
27-30: Import update looks good.The added
is_prow_environmentimport is clean and keeps the new guard logic explicit.
| if is_prow_environment(): | ||
| return False |
There was a problem hiding this comment.
RUNNING_PROW presence check can incorrectly disable Docker mode.
On Line 39, is_prow_environment() is truthy for any defined value (including empty string), so Line 40 can force False outside real Prow runs. That can mis-set context.is_docker_mode for the whole scenario.
Proposed fix (in tests/e2e/utils/utils.py)
def is_prow_environment() -> bool:
"""Check if running in Prow/OpenShift environment."""
- return os.getenv("RUNNING_PROW") is not None
+ value = os.getenv("RUNNING_PROW")
+ return value is not None and value.strip().lower() in {"1", "true", "yes", "on"}Based on learnings: In the Behave Python testing framework, the Context object is created once for the entire test run and reused across all features and scenarios. Custom attributes added to the context persist until explicitly cleared, so per-scenario state should be reset in hooks to maintain test isolation.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/e2e/features/steps/proxy.py` around lines 39 - 40, The
is_prow_environment() check is treating any defined RUNNING_PROW value
(including empty string) as truthy and can incorrectly force
context.is_docker_mode False; update is_prow_environment() to return a strict
boolean by reading the env var (e.g., os.getenv("RUNNING_PROW") in utils and
compare to a specific expected value like "1" or "true") and change the proxy
step to rely on that strict boolean; additionally, ensure per-scenario state is
reset by clearing or reinitializing context.is_docker_mode in the Behave
before_scenario hook (or equivalent) so the Docker mode decision
(context.is_docker_mode) cannot leak between scenarios.
2 participants
Description
Type of change
Tools used to create PR
Identify any AI code assistants used in this PR (for transparency and review context)
Related Tickets & Documents
Checklist before requesting a review
Testing
Summary by CodeRabbit