Docker: Retain recordings for failed sessions only from session capabilities#3111
Merged
Docker: Retain recordings for failed sessions only from session capabilities#3111
Conversation
Contributor
Review Summary by QodoImplement retain-on-failure strategy with session capability override
WalkthroughsDescription• Implement retain-on-failure strategy to discard passing session recordings • Refactor upload logic to support artifact-agnostic queuing and processing • Add session capability se:retainOnFailure for per-session override control • Expand default failure event detection to include :error and :aborted • Update environment variable naming for clarity: SE_UPLOAD_FAILURE_SESSION_ONLY → SE_RETAIN_ON_FAILURE Diagramflowchart LR
SessionCreated["Session Created<br/>with capability"] -->|Check se:retainOnFailure| CapCheck{"Capability<br/>set?"}
CapCheck -->|Yes| UseCap["Use capability<br/>value"]
CapCheck -->|No| UseEnv["Use SE_RETAIN_ON_FAILURE<br/>env var"]
UseCap --> SessionState["Store in<br/>SessionState"]
UseEnv --> SessionState
SessionState -->|Recording stops| FailureCheck{"Session<br/>failed?"}
FailureCheck -->|Yes| Queue["Queue for upload"]
FailureCheck -->|No| Discard["Discard video<br/>immediately"]
Queue --> Upload["Upload artifact"]
Discard --> Cleanup["Cleanup session"]
File Changes1. Video/video_service.py
|
Contributor
Code Review by Qodo
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Comment on lines
+259
to
+260
| if [ -n "${TEST_RETAIN_ON_FAILURE}" ]; then | ||
| echo "TEST_RETAIN_ON_FAILURE=${TEST_RETAIN_ON_FAILURE}" >> $GITHUB_ENV |
Contributor
There was a problem hiding this comment.
1. Unquoted $github_env redirection 📘 Rule violation ☼ Reliability
The new workflow line appends to $GITHUB_ENV without quoting the path, which is brittle and can break if the path contains spaces or special characters. This violates the requirement to quote variables/paths in shell scripts to avoid word-splitting/globbing issues.
Agent Prompt
## Issue description
The workflow appends to `GITHUB_ENV` using an unquoted path (`>> $GITHUB_ENV`), which is brittle under shell word-splitting rules.
## Issue Context
GitHub Actions recommends treating `GITHUB_ENV` as a file path; quoting prevents failures if the path ever contains spaces/special characters.
## Fix Focus Areas
- .github/workflows/docker-test.yml[259-261]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+143
to
148
| if TEST_RETAIN_ON_FAILURE: | ||
| self.driver.fire_session_event( | ||
| "test:failed", | ||
| {"test": f"{self._testMethodName} ({self.__class__.__name__})"}, | ||
| ) | ||
| self.driver.quit() |
Contributor
There was a problem hiding this comment.
2. Failure event always fired 🐞 Bug ≡ Correctness
SeleniumGenericTests.tearDown() fires a "test:failed" session event whenever TEST_RETAIN_ON_FAILURE is enabled, even for passing tests, so sessions are always marked failed and recordings will never be discarded. This makes CI and local runs unable to validate retain-on-failure behavior and can cause unnecessary retention/uploads.
Agent Prompt
### Issue description
`tests/SeleniumTests/__init__.py` fires `driver.fire_session_event("test:failed", ...)` in `tearDown()` whenever `TEST_RETAIN_ON_FAILURE` is enabled, regardless of whether the test actually failed. This forces the video service to mark all sessions as failed and prevents retain-on-failure from ever discarding videos for passing sessions.
### Issue Context
The video service treats event types containing `:failed` as failures by default, so emitting that event for successful tests invalidates the new retain-on-failure behavior and produces misleading CI artifacts.
### Fix Focus Areas
- tests/SeleniumTests/__init__.py[136-148]
### Suggested change
Update `tearDown()` to emit the failure session event **only when the test failed/error’d**. In `unittest`, you can inspect `self._outcome` / `self._outcome.result` (failures + errors) to determine whether the current test had an exception, and only then call `fire_session_event("test:failed", ...)` before `quit()`.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
VietND96
force-pushed
the
retain-on-failure
branch
2 times, most recently
from
639ff8d to
f606d8c
Compare
…ilities Signed-off-by: Viet Nguyen Duc <nguyenducviet4496@gmail.com>
VietND96
force-pushed
the
retain-on-failure
branch
from
f606d8c to
a5fee57
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Thanks for contributing to the Docker-Selenium project!
A PR well described will help maintainers to quickly review and merge it
Before submitting your PR, please check our contributing guidelines, applied for this repository.
Avoid large PRs, help reviewers by making them as simple and short as possible.
Description
Retain recordings for failed sessions only
In event-driven mode (
SE_VIDEO_EVENT_DRIVEN=true, the default), the video service subscribes to the Grid's ZeroMQ event bus and reacts to session lifecycle events in real time. This enables a retain-on-failure strategy: record every session, but automatically discard the video when the session passes and only keep (and upload) recordings from sessions that fail.Enable it globally with the environment variable:
SE_RETAIN_ON_FAILURE=trueA session is treated as failed when either of the following is true:
eventTypecontains a substring fromSE_FAILURE_SESSION_EVENTS(default::failed,:failure,:error,:aborted).TIMEOUT,NODE_REMOVED, orNODE_RESTARTED— instead of the normalQUIT_COMMAND.SE_RETAIN_ON_FAILUREfalseSE_FAILURE_SESSION_EVENTS:failed,:failure,:error,:abortedeventTypecontains one of these (case-insensitive) marks the session as failed.The
se:retainOnFailuresession capability overrides the global container env var for a specific session. For example, to retain the recording of a single session regardless of the global setting:se:retainOnFailurecapSE_RETAIN_ON_FAILUREenvtruefalse(default)falsetruetruefalse(default)Firing session events from test code
The Session Event API lets test code push named events directly to the Grid. The video service listens for these events on the ZeroMQ bus and uses them to determine session failure.
Call
driver.fire_session_event(eventType, payload)from your test. AnyeventTypethat contains a configured failure substring (e.g."test:failed"contains":failed") marks the session as failed.So, you can control the retain-on-failure strategy fully from test code via session capabilities and fire session event.
Motivation and Context
Types of changes
Checklist