Conversation
The shadow-pierce scripts are IIFE expressions, so the Selenium wrapper built a function body that evaluated the IIFE and discarded it, making find_first, find_all and assert_pierced_visible resolve to undefined against every real driver. The Playwright branch already returned its expression. Wrap the expression in `return ( ... )` rather than prefixing `return`: the scripts open with a newline, so a bare `return` would be closed by automatic semicolon insertion and keep yielding undefined. The existing unit tests stub execute_script on a MagicMock and never evaluate the script, so add a guard on the emitted script shape that rejects both failure modes.
data: URLs are opaque origins, so Chrome rejects every localStorage access there with "Storage is disabled inside 'data:' URLs" and capture_state could never complete. Add a storage_origin fixture that parks the shared driver on a genuine http origin, configurable through WEBRUNNER_E2E_ORIGIN and defaulting to the hub's Grid console, which is the one http server the browser container is guaranteed to reach. Skip instead of failing when no such origin is available, and drop the key afterwards so the session-scoped driver doesn't leak state into the tests that follow.
|
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 0 |
| Duplication | 0 |
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
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.
The scheduled E2E Browser Smoke job has failed on every run since it was
added, 3 of 6 tests each time. Two independent causes, verified against a real
headless Chrome.
1.
shadow_piercenever returned its result on Selenium_execute_jsbuilt a function body out of an IIFE expression and dropped thevalue, so
find_first,find_allandassert_pierced_visibleresolved toundefinedagainst every real Selenium driver — not just in CI. The Gridlog shows the tell:
execute/syncansweringHTTP 200with{"value":null},i.e. the script ran fine and returned nothing. The Playwright branch already
returned its expression correctly.
Fixed by wrapping in
return ( ... ). Prefixing a barereturnis not enough:the scripts open with a newline, so automatic semicolon insertion turns it into
return;and the result is stillundefined.The existing unit tests could not catch this — they stub
execute_scripton aMagicMock, so the JavaScript is never evaluated. Added a guard on the emittedscript shape that rejects both failure modes. This matters because the E2E job
is scheduled-only and does not gate PRs.
2. Storage tests ran on a
data:URLdata:URLs are opaque origins; Chrome rejects everylocalStorageaccess with"Storage is disabled inside 'data:' URLs", so
capture_statecould nevercomplete. Not a library defect —
state_diffbuilds its script correctly.Added a
storage_originfixture that parks the shared driver on a genuine httporigin, configurable via
WEBRUNNER_E2E_ORIGIN, defaulting to the hub's Gridconsole — the one http server the browser container is guaranteed to reach
(
localhost:4444is meaningful to the runner, not to Chrome inside the nodecontainer). It skips rather than fails when no origin is available, and drops
the key afterwards so the session-scoped driver stays clean.
Verification
test/unit_test/test_*.pypytest test/e2e_test/ -m e2ewith no Gridreturnpatternshadow_pierce;cookie_consentandbrowser_recorderare correctThe new guard was confirmed to fail on both the original code and the naive
return-prefixed variant, and to pass only on the fix.🤖 Generated with Claude Code