Repository navigation
Let libraries trace and record adopted contexts the way New Context does - #5326
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
Adopted HAR options can forward string booleans to Playwright without conversion.
1 open finding
What changed in this PR
Adds public helpers for libraries adopting Browser contexts while aligning tracing, video, and HAR behavior with New Context.
Changes:
- Adds adopted-context option generation and registration.
- Resolves relative HAR paths against the output directory.
- Adds unit and acceptance coverage.
| File | Description |
|---|---|
Browser/browser.py |
Adds adopted-context APIs and shortens extension help. |
Browser/keywords/playwright_state.py |
Resolves relative HAR paths. |
Browser/utils/data_types.py |
Defines adopted-context option types. |
Browser/utils/__init__.py |
Exports the new type. |
utest/test_adopted_context.py |
Tests options and registration. |
atest/test/01_Browser_Management/record_har.robot |
Verifies relative HAR paths. |
🧠 Review effort: Balanced
aaltat
left a comment
aaltat
left a comment
There was a problem hiding this comment.
Thanks for the follow-up, Daniel. Some comments inline and two more here.
#5152: the change to relative recordHar paths fixes #5152. Could you add Fixes #5152 to the PR description, so that the issue is closed and the change shows up in the release notes?
Missing tests:
- Most important: an acceptance test for the whole flow.
adopt_context.robotdoes not useadopted_context_optionsorregister_adopted_context, so right now only the Electron library tests the path from the extension to registration. For example, a small library inatest/library/that subclassesBrowser, callsadopted_context_options, adopts a context throughadopt.jswith tracing and video, and callsregister_adopted_context. Then the test checks that the trace has the keyword groups and that the video is written. - Unit tests for:
- a relative trace folder (not a
.zipfile) - an absolute video
dir - the Robot Framework variable
${ROBOT_FRAMEWORK_BROWSER_TRACING}(only the environment variable is tested now) TracingGroupMode.Playwright, which should not add trace groups- a relative HAR path with
New Persistent Context
- a relative trace folder (not a
|
|
||
|
|
||
| class NoPlaywright: | ||
| """Stands in for the Node process, which these tests do not need.""" |
There was a problem hiding this comment.
Please remove this docstring. We avoid docstrings and comments in tests; the class name already says what it is.
There was a problem hiding this comment.
Removed. The fake now also stands in for the gRPC stub, so it is called FakePlaywright.
| ) | ||
| if not options["recordVideo"]: | ||
| return "" | ||
| state.context_cache.add(context_id, options["recordVideo"]["size"]) |
There was a problem hiding this comment.
This repeats the end of new_context and new_persistent_context in playwright_state.py (add the context to the trace, context_cache.add, _embed_video). Could you move it into one helper on PlaywrightState that all three use? Then the next change to how a context is registered only has to happen in one place.
There was a problem hiding this comment.
Done. _register_context on PlaywrightState adds the trace groups, the context cache entry and the video, and New Context, New Persistent Context and register_adopted_context use it. New Persistent Context calls it right after the context is created, where it added the trace groups before, so that the navigation to url stays inside the keyword's group in the trace.
| "recordHar": None, | ||
| } | ||
| if recordVideo is not None: | ||
| params = {"recordVideo": {"dir": None, **recordVideo}} |
There was a problem hiding this comment.
New Context takes the video size from the viewport when recordVideo has no size, and only then falls back to 1280x720 (_get_video_size). Here only recordVideo is used, so an adopted context with a custom viewport gets 1280x720. Could you make it follow the same rule as New Context?
There was a problem hiding this comment.
Done. adopted_context_options takes an optional viewport, and a video without a size gets its size, as in New Context, or 1280x720.
| "recordVideo": None, | ||
| "recordHar": None, | ||
| } | ||
| if recordVideo is not None: |
There was a problem hiding this comment.
New Context runs its arguments through convert_typed_dict, so the keys of recordVideo and recordHar are matched case-insensitively (Dir, Size, ...). adopted_context_options skips that, so {"Dir": ...} works with New Context but not here. Please convert them the same way.
There was a problem hiding this comment.
I looked into this before changing it, and New Context does not match these keys case-insensitively today:
- Robot Framework rejects
New Context recordVideo={'Dir': 'videos'}before the keyword runs, because it checks the keys of theRecordVideoTypedDict:Argument 'recordVideo' got value '{'Dir': 'videos'}' that cannot be converted to RecordVideo or None.(tried with RF 7.5). - From Python,
convert_typed_dictconvertsrecordVideoonly on Python 3.14. Before 3.14,RecordVideo | Noneis atypes.UnionType, which has no__origin__, so the check forUniondoes not match and the dictionary is passed on as it is. That is also whyvideo.robotneeded${PYTHON_314}. - On 3.14 the conversion keeps only the keys that
RecordVideodeclares. Playwright'srecordVideoalso hasshowActions, the overlay of the performed actions, and the Electron library passes it throughadopted_context_options. With the conversion it would be lost, but only on Python 3.14.
My suggestion: add showActions to RecordVideo, as a small ShowActions TypedDict with duration, position, fontSize and cursor, as Playwright documents it for the context options. Then New Context and New Persistent Context get the overlay too, the conversion keeps it, and AdoptedContextOptions can use RecordVideo and RecordHar (see the other thread). Shall I add it to this PR, or in a separate one?
| return None | ||
| return json.loads(last_json) | ||
|
|
||
| def adopted_context_options( |
There was a problem hiding this comment.
When Browser launches a browser, it sets tracesDir to a unique folder under ${OUTPUT_DIR}/browser/traces/temp (_set_browser_options), and Browser removes these temporary trace files. The options here do not include tracesDir, so when an extension launches its own browser or Electron app with tracing, the temporary trace files end up outside that folder. Could adopted_context_options return tracesDir too, so that the extension can pass it to the launch?
There was a problem hiding this comment.
Done. The options include tracesDir, a new folder under browser/traces/temp, as New Browser uses for the browsers it launches, and the docstring says to pass it to the launch. The Electron library passes it to _electron.launch, and the new acceptance test passes it to launchPersistentContext.
| The keys can be passed to the extension function as keyword arguments: | ||
| ``tracing`` for the ``tracing`` option of ``adoptContext``, ``recordVideo`` | ||
| and ``recordHar`` for Playwright when the extension creates the context. | ||
| After the extension returns, call `register_adopted_context` with these |
There was a problem hiding this comment.
In libdoc, single backticks link to a keyword, but register_adopted_context (here) and adopted_context_options (line 951) are not keywords, so the links point nowhere. Please use double backticks for both.
There was a problem hiding this comment.
Done, both are in double backticks now.
| """ | ||
|
|
||
| tracing: str | None | ||
| recordVideo: dict[str, Any] | None |
There was a problem hiding this comment.
Why dict[str, Any] here, when RecordVideo and RecordHar already exist and are used for the arguments of adopted_context_options? Let's discuss this one.
There was a problem hiding this comment.
Because of showActions: the Electron library passes Playwright's recordVideo.showActions through adopted_context_options, and RecordVideo does not declare it, so typing the returned dictionary as RecordVideo would hide a key that is there. If RecordVideo gets showActions, as I suggested in the thread about the case-insensitive keys, AdoptedContextOptions can use RecordVideo | None and RecordHar | None, and I will change it then. Until we agree on that, I left dict[str, Any].
There was a problem hiding this comment.
I agree with your suggestion. I will merge this PR and send time on fixing few other minor things in the same go.
|
Daniel, one more, and it is the bigger one. Here is a sketch of what I mean. It is not tested; names and details are up to you. 1. The helpers take the option dict instead of the whole # Browser/keywords/playwright_state.py
def _record_video(self, record_video: dict | None, viewport: dict | None) -> dict | None:
if not record_video:
return None
video_dir = record_video.get("dir")
if video_dir is None:
resolved_dir = self.video_output
elif Path(video_dir).is_absolute():
resolved_dir = Path(video_dir)
else:
resolved_dir = self.video_output / video_dir
return {
**record_video,
"dir": str(resolved_dir.resolve()),
"size": self._video_size(record_video, viewport),
}
def _video_size(self, record_video: dict | None, viewport: dict | None) -> dict:
size = (record_video or {}).get("size") or {}
if size.get("width") and size.get("height"):
return {"width": int(size["width"]), "height": int(size["height"])}
return viewport or {"width": 1280, "height": 720}
def _record_har(self, record_har: dict | None) -> dict | None:
if not record_har:
return None
if not record_har.get("path"):
return {**record_har}
return {**record_har, "path": str(Path(self.outputdir, record_har["path"]).resolve())}2. One registration helper, shared by def _register_context(
self, context_id: str, trace_file: str, video_size: dict, video: dict | None = None
) -> str:
self.add_context_and_keyword_call_stack_to_trace(trace_file, context_id)
self.context_cache.add(context_id, video_size)
return self._embed_video(video) if video else ""3. def _set_context_options(self, params, httpCredentials, storageState):
params = convert_typed_dict(self.new_context.__annotations__, params)
params["recordVideo"] = self._record_video(params.get("recordVideo"), params.get("viewport"))
params["recordHar"] = self._record_har(params.get("recordHar"))
... # reducedMotion, storageState, httpCredentials as today
# end of new_context (_new_context no longer adds the trace groups itself):
size = self._video_size(params.get("recordVideo"), params.get("viewport"))
self._register_context(response.id, trace_file, size)
return response.id
# end of new_persistent_context:
size = self._video_size(params.get("recordVideo"), params.get("viewport"))
video_path = self._register_context(
response.id, trace_file, size, json.loads(response.video)
)
4. The adopted path on def adopted_context_options(
self, tracing=None, recordVideo=None, recordHar=None, viewport=None
) -> AdoptedContextOptions:
params = convert_typed_dict( # case-insensitive keys, as in New Context
self.new_context.__annotations__,
{k: v for k, v in {"recordVideo": recordVideo, "recordHar": recordHar}.items() if v},
)
return {
"tracing": self._resolve_trace_file(tracing) or None,
"recordVideo": self._record_video(params.get("recordVideo"), viewport),
"recordHar": self._record_har(params.get("recordHar")),
"tracesDir": str(self.traces_temp / str(uuid4())), # as in _set_browser_options
}
def register_adopted_context(
self, context_id: str, options: AdoptedContextOptions, video_path: str | None = None
) -> str:
record_video = options["recordVideo"]
video = {"video_path": video_path, "contextUuid": context_id} if record_video else None
size = record_video["size"] if record_video else self._video_size(None, None)
return self._register_context(context_id, options["tracing"] or "", size, video)This also covers the inline comments about the viewport, the case-insensitive keys, 5. # Browser/browser.py
def adopted_context_options(
self,
tracing: bool | Path | None = None,
recordVideo: RecordVideo | None = None,
recordHar: RecordHar | None = None,
viewport: ViewportDimensions | None = None,
) -> AdoptedContextOptions:
"""Docstring for library authors, as now."""
return self._playwright_state.adopted_context_options(
tracing, recordVideo, recordHar, viewport
)
def register_adopted_context(
self, context_id: str, options: AdoptedContextOptions, video_path: str | None = None
) -> str:
"""Docstring for library authors, as now."""
return self._playwright_state.register_adopted_context(context_id, options, video_path)About the new What do you think? |
|
Generally I agree on your idea, wrote few comments to fix. |
New Context and New Persistent Context passed a relative recordHar path to the Node process as given, so the HAR file ended up in Browser's own wrapper folder. A relative path is now relative to the output directory, as a .zip path of tracing is.
A library that hands over its own context with adoptContext had to reach into Browser's internals to trace and record it the way New Context does. - adopted_context_options returns tracing, recordVideo and recordHar by the rules of New Context, including ROBOT_FRAMEWORK_BROWSER_TRACING and Browser's trace and video folders. - register_adopted_context registers the adopted context afterwards, so that the keyword calls become trace groups and the log embeds the video.
adopted_context_options returns a typed dictionary with tracing, recordVideo and recordHar, None when off, so that a library can pass it on to the extension function as keyword arguments. Its parameters take Browser's RecordVideo and RecordHar, as New Context does.
adopted_context_options and register_adopted_context call PlaywrightState
now, where New Context and New Persistent Context resolve their recordVideo
and recordHar options and register their contexts with the same helpers.
Browser keeps only the two public methods.
- adopted_context_options takes the viewport of the adopted context, which
gives the video its size when recordVideo has none, as in New Context. It
also returns tracesDir, a new folder for Playwright's temporary trace
files, as New Browser gives one to each browser it launches.
- Only None means off, so an empty recordVideo records with the defaults.
New Context recordVideo={} got no dir before, and Playwright recorded into
its temporary artifacts folder, which it deletes when the browser closes.
- New Context no longer changes the caller's recordVideo. Before Python
3.14, dir became an absolute PosixPath in it; video.robot checked that per
Python version and no longer needs ${PYTHON_314}.
- New Persistent Context registers the context right after creating it,
where it added the trace groups before, so that the navigation to url
stays inside the keyword's trace group.
AdoptingLibrary subclasses Browser, adopts a persistent context through adopt.js with the options of adopted_context_options and registers it with register_adopted_context. The test checks that the trace has the groups of the keywords and that the video is written to Browser's video folder.
b100dee to
0c42907
Compare
|
Thanks for the review, Tatu. I followed your sketch: the logic is in
Tests:
I added |
|
Some flaky tests, eh? :-) The four red jobs fail in tests this PR doesn't touch: |
|
Yes, we have flaky test and have been working to improve the situation, but clearly the work is not yet completed. I think your analysis is correct and all failures are old stuff and I did start rerun. Let’s see what the results says. |
|
I wrote a ticket about the flaky test which fixes did not work yet #5343 |
|
Rerunning did help, I will look into this later, can’t promise today but at least in the train tomorrow. |
Browser merged the methods for adopted contexts in MarketSquare/robotframework-browser#5326, so the fork branch is no longer needed. Until a Browser release contains them, CI and the local clone use Browser's main, which also has the maintainers' follow-ups.
Follow-up to #5318, as discussed there. Fixes #5152.
With #5318, Browser starts tracing an adopted context, but a library still has to work out the trace path, the video folder and size and the HAR path itself, and register the context for the keyword groups in the trace and for the video in the log. The Electron library did that through internals of
PlaywrightState, such as_resolve_trace_file,_set_video_pathand_embed_video. This PR gives libraries built on Browser two public methods for it, so that an adopted context follows Browser's settings in the same way asNew Context.Browser.adopted_context_options(tracing=None, recordVideo=None, recordHar=None, viewport=None)returns anAdoptedContextOptions, a typed dictionary withtracing,recordVideo,recordHarandtracesDir. They follow the rules ofNew Context:tracing=TrueandROBOT_FRAMEWORK_BROWSER_TRACINGgive Browser's trace folder, a video gets Browser's video folder and the size ofviewportor 1280x720 by default, an emptyrecordVideorecords with these defaults, and relative paths are resolved as forNew Context.Nonemeans off.tracesDiris a new folder for Playwright's temporary trace files underbrowser/traces/temp, asNew Browsergives one to each browser it launches. The keys are named like the options, so a library can pass them on to its extension function as keyword arguments.Browser.register_adopted_context(context_id, options, video_path=None)registers the adopted context afterwards: the keyword calls become groups in the trace, and the log embeds the video. It returns the video path, or an empty string without a video.Both call
PlaywrightState, whereNew ContextandNew Persistent Contextresolve their options and register their contexts with the same helpers.A library in Python then does:
Related changes:
recordHarpaths (New Context recordHar with a relative path writes the HAR into the wrapper directory, also on the Robot Framework path #5152):New ContextandNew Persistent Contextpassed thepathofrecordHarto the Node process as given, so a relative path put the HAR file into thewrapperfolder of the installed package. A relative path is now relative to the output directory, as a.zippath oftracingis, andadopted_context_optionsuses the same rule. UnlikerecordVideoandtracing,recordHarhas no default location yet. Browser could give it one, for examplebrowser/har/<unique name>.harwhen thepathis left out, and adopted contexts would get it throughadopted_context_optionstoo. I left that out to keep this PR small, but I can add it here or in a separate PR if you like the idea.recordVideo:New Context recordVideo={}got nodir, so Playwright recorded into its temporary artifacts folder and deleted the video when the browser closed, while the log still linked to it. It now records into Browser's video folder.recordVideostays as given: before Python 3.14,New Contextchangeddirin the caller's dictionary to an absolutePosixPath. It now works on a copy on every Python version.The JavaScript extensions page describes the two methods, updated in MarketSquare/robotframework-browser.org#21.
Tests:
atest/test/05_JS_Tests/adopted_context_recording.robot: a small library built on Browser,atest/library/AdoptingLibrary.py, adopts a persistent context throughadopt.jswith the options ofadopted_context_optionsand registers it. The trace has the keyword groups, and the video is written to Browser's video folder.utest/test_adopted_context.py: the options followNew Context, including the trace folders, the global tracing setting as environment variable and as Robot Framework variable, the video folder and size, the viewport,tracesDirand the HAR paths. Registering opens the trace groups, except in the tracing group modePlaywright, and embeds the video.New ContextandNew Persistent Contextsend relative HAR paths resolved.atest/test/01_Browser_Management/record_har.robot: a relative HAR path ends up in the output directory.atest/test/01_Browser_Management/video.robot: the caller'srecordVideostays as given on every Python version.There are no Node or protocol changes. I verified it locally with
inv lint-python,inv lint-robot,inv lint-node,inv utestand the acceptance suitesAdopted Context Recording,Adopt Context,Record Har,Tracing,Tracing Groups,Video,Persistent State,Playwright State,GeolocationandBrowser New Context Test, with Chromium only. The Electron library uses the two methods and passestracesDirto_electron.launch, and its tests pass against this branch.AI / tooling disclosure
This contribution was prepared with assistance from Claude Code (Anthropic). I reviewed the result manually, understand the changes, verified them locally, and confirm that I have the right to submit this contribution under the project license.