Skip to content

Workarena/Base: Merge init scripts before evaluation.#83

Open
conte91 wants to merge 3 commits intoServiceNow:mainfrom
conte91:js_init
Open

Workarena/Base: Merge init scripts before evaluation.#83
conte91 wants to merge 3 commits intoServiceNow:mainfrom
conte91:js_init

Conversation

@conte91
Copy link

@conte91 conte91 commented Jul 4, 2025

Fixes #57 and duplicates without the need for downgrading the playwright version.

Playwright's init scripts are not executed sequentially, as per PW's docs:

image

However, some benchmarks depend on all installed init scripts to be executed in order, which is why the page hangs if they don't (in WorkArena's case, this was because of the function being waited on being undefined - since the script defining it hadn't been evaluated yet).

It shouldn't cause any problem to just concatenate all scripts together, so that we can guarantee FIFO execution order.

@conte91 conte91 marked this pull request as draft July 4, 2025 17:20
Comment on lines 155 to 157
page.evaluate(init_script_js)
for f in page.frames:
f.evaluate(init_script_js)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the evaluate lines should be removed

@conte91 conte91 requested a review from younik July 11, 2025 11:09
@conte91 conte91 changed the title Workarena/Base: Add init scripts a single JS file in all page/frames. Workarena/Base: Merge init scripts before evaluation. Jul 16, 2025
@conte91 conte91 marked this pull request as ready for review July 16, 2025 18:31
@conte91
Copy link
Author

conte91 commented Jul 16, 2025

Self-note: this could be an issue in case names defined across scripts shadow each other?

Something like:

add_init_script(`const Math = () => {alert("fake math!");}`)
add_init_script(`Math.random()`)

I think we should be fine though, because even if two scripts' private functions will share the same name, the last one defined will win.

@aldro61
Copy link
Collaborator

aldro61 commented Feb 20, 2026

Hi, thanks for the PR @conte91, @younik. This works for most tasks but currently breaks the chart-related ones. I'm looking into what the problem could be.

@aldro61
Copy link
Collaborator

aldro61 commented Feb 23, 2026

@conte91 Found a regression introduced by this PR — chart tasks (SingleChartValueRetrievalTask, MultiChartValueRetrievalTask, etc.) fail with ReferenceError: findElementInShadowDOM is not defined because merging all scripts into a single string prevents function declarations from being globally accessible when called from outside the init script context (e.g. via iframe.evaluate_handle). Note that window.WORKARENA_LOAD_COMPLETE = true still works because it's an explicit window. assignment — only bare function declarations are affected.

Fix is to add these 3 lines to js_utils.js right after the findElementInShadowDOM function definition (see:

// Explicitly expose on window so it is accessible via frame.evaluate_handle()
// when all init scripts are merged into a single string (see base.py).
window.findElementInShadowDOM = findElementInShadowDOM;
):

// Explicitly expose on window so it is accessible via frame.evaluate_handle()
// when all init scripts are merged into a single string (see base.py).
window.findElementInShadowDOM = findElementInShadowDOM;

Verified with test_task_general.py (all 165 tests passing, including all chart tasks) and a sample of L2 compositional tasks including DashboardRetrieve* tasks.

Please add these 3 lines and i'll merge as soon as this is included!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: Hitting Timeouts error almost systematically

3 participants