Skip to content
This repository was archived by the owner on Jan 30, 2025. It is now read-only.

Fix error for evaluate APIs #1233

Merged
merged 3 commits into from
Mar 5, 2024
Merged

Fix error for evaluate APIs #1233

merged 3 commits into from
Mar 5, 2024

Conversation

ankur22
Copy link
Collaborator

@ankur22 ankur22 commented Mar 5, 2024 •
edited
Loading

What?

This fixes the error that is returned when a user works with the evaluate APIs when a navigation isn't occurring:

  const page = browser.newPage();

  const blah = page.evaluate("1+1");
  console.log(blah);
  
  page.close();

This used to return GoError: evaluating JS: execution context changed; most likely because of a navigation, but now it returns given expression does not evaluate to a function.

Why?

The new error better represents what the user can do to fix the problem instead of being left confused when no navigation has occurred. In the example above and with the new error, they should be able to work out that evaluate only works with callable functions.

Checklist

  • I have performed a self-review of my code
  • I have added tests for my changes
  • I have commented on my code, particularly in hard-to-understand areas

Related PR(s)/Issue(s)

https://github.com/grafana/k6-cloud/issues/2066

@ankur22 ankur22 requested a review from inancgumus March 5, 2024 09:20
inancgumus
inancgumus previously approved these changes Mar 5, 2024
View reviewed changes
Copy link
Collaborator

@inancgumus inancgumus left a comment •
edited
Loading

Choose a reason for hiding this comment

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

LGTM 👍

Some suggestions.

There is also a linter error:

TestPageEvaluateMapping should call t.Parallel on the top level as well as its subtests (parallel)

ankur22 added a commit that referenced this pull request Mar 5, 2024
@ankur22
Fix error handling from chromium
9277d60 
When the browser module receives the error with code -32000, this
indicates a server error. From looking at the chromium code it seems to
be related to issues that originate not from chromium but from the
caller. There are many error messages that use this error code, so we
cannot assume that it is due to a navigation. The user and we will
benefit from knowing what chrome/chromium is not happy about

Resolves: #1233 (comment)
ankur22 added a commit that referenced this pull request Mar 5, 2024
@ankur22
Fix error handling from chromium
9ef80b3 
When the browser module receives the error with code -32000, this
indicates a server error. From looking at the chromium code it seems to
be related to issues that originate not from chromium but from the
caller. There are many error messages that use this error code, so we
cannot assume that it is due to a navigation. The user and we will
benefit from knowing what chrome/chromium is not happy about

Resolves: #1233 (comment)
@ankur22 ankur22 force-pushed the add/not-func-error branch from 9277d60 to 9ef80b3 Compare March 5, 2024 12:05
ankur22 added a commit that referenced this pull request Mar 5, 2024
@ankur22
Fix error handling from chromium
f927336 
When the browser module receives the error with code -32000, this
indicates a server error. From looking at the chromium code it seems to
be related to issues that originate not from chromium but from the
caller. There are many error messages that use this error code, so we
cannot assume that it is due to a navigation. The user and we will
benefit from knowing what chrome/chromium is not happy about

Resolves: #1233 (comment)
@ankur22 ankur22 force-pushed the add/not-func-error branch from 9ef80b3 to f927336 Compare March 5, 2024 12:07
ankur22 added 3 commits March 5, 2024 14:25
@ankur22
Add correct error when evaluate script not func
05c7ff9 
The error message otherwise is confusing if no navigations occurred
during the test. Taking a look at the actual error it was due to the
script not being a function/callable.
@ankur22
Add test for page.evaluate
a5fc491 
This integration test is for sanity checking that it does indeed work
as we expect when we pass it the correct function.
@ankur22
Fix error handling from chromium
0b50d16 
When the browser module receives the error with code -32000, this
indicates a server error. From looking at the chromium code it seems to
be related to issues that originate not from chromium but from the
caller. There are many error messages that use this error code, so we
cannot assume that it is due to a navigation. The user and we will
benefit from knowing what chrome/chromium is not happy about

Resolves: #1233 (comment)
@ankur22 ankur22 force-pushed the add/not-func-error branch from f927336 to 0b50d16 Compare March 5, 2024 14:25
@ankur22 ankur22 requested a review from inancgumus March 5, 2024 14:26
@ankur22 ankur22 merged commit 57f169e into main Mar 5, 2024
@ankur22 ankur22 deleted the add/not-func-error branch March 5, 2024 14:58
@BrewTestBot BrewTestBot mentioned this pull request Mar 25, 2024
Merged
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

1 more reviewer

@inancgumus inancgumus inancgumus approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@ankur22 @inancgumus