Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

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
Copy link
Member

@inancgumus inancgumus left a comment

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)

common/execution_context.go Outdated Show resolved Hide resolved
tests/page_test.go Outdated Show resolved Hide resolved
tests/page_test.go Outdated Show resolved Hide resolved
ankur22 added a commit that referenced this pull request Mar 5, 2024
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
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
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
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.
This integration test is for sanity checking that it does indeed work
as we expect when we pass it the correct function.
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
17 checks passed
@ankur22 ankur22 deleted the add/not-func-error branch March 5, 2024 14:58
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.

2 participants