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

Add page.on #1006

Merged
merged 11 commits into from
Aug 29, 2023
Merged

Add page.on #1006

merged 11 commits into from
Aug 29, 2023

Conversation

ka3de
Copy link
Collaborator

@ka3de ka3de commented Aug 21, 2023

What?

Adds support for page.on method. Currently 'console' event is the only one allowed.

This will allow users to define a function to be executed every time a console API is called in the web page context. Such as:

  • console.log
  • console.debug
  • console.info
  • console.error
  • console.warn
  • console.dir
  • console.dirxml
  • console.table
  • console.trace
  • console.clear
  • console.assert
  • console.profile
  • console.profileEnd
  • console.group
  • console.groupCollapsed
  • console.groupEnd
  • console.count

See full reference and details at MDN Web Docs for console API.

The handler function defined by the user will receive a console message object as input for every call, such as:

ConsoleMessage {
    args()    // returns the input arguments for the console API method call
    page()   // returns the page that produced this console message
    text()     // returns the text for the console message
    type()    // returns the type for the console message
}

Why?

  • Increases the compatibility with Playwright.
  • Allows users to perform assertions on console events from the page context execution.

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)

Updates #835.

@ka3de ka3de force-pushed the feat/835-page-on-console branch 2 times, most recently from ec8c47b to dbe13fb Compare August 22, 2023 10:13
@ka3de ka3de mentioned this pull request Aug 23, 2023
1 task
ka3de added a commit that referenced this pull request Aug 23, 2023
This is added temporarily until these methods are used in a related PR
(see #1006).
@ka3de ka3de force-pushed the feat/835-page-on-console branch from dbe13fb to cb5140c Compare August 23, 2023 10:43
ka3de added a commit that referenced this pull request Aug 23, 2023
This is added temporarily until these methods are used in a related PR
(see #1006).
@ka3de ka3de force-pushed the feat/835-page-on-console branch 2 times, most recently from b69b0df to e61b8e2 Compare August 23, 2023 10:55
ka3de added a commit that referenced this pull request Aug 23, 2023
This is added temporarily until these methods are used in a related PR
(see #1006).
ka3de added a commit that referenced this pull request Aug 23, 2023
This is added temporarily until these methods are used in a related PR
(see #1006).
@ka3de ka3de force-pushed the feat/835-page-on-console branch 11 times, most recently from dd20994 to 8c3118e Compare August 25, 2023 09:59
common/page.go Show resolved Hide resolved
@ka3de ka3de marked this pull request as ready for review August 25, 2023 10:20
@ka3de ka3de requested review from inancgumus and ankur22 August 25, 2023 10:20
@ka3de ka3de force-pushed the feat/835-page-on-console branch from 8c3118e to 6d2e50b Compare August 25, 2023 10:44
inancgumus
inancgumus previously approved these changes Aug 25, 2023
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.

Excellent work 👏 Thank you for working on this challenging task and delivering it quite remarkably 🙇 I believe we might want to refactor the TaskQueue part later, but it's okay for now.

tests/page_test.go Outdated Show resolved Hide resolved
common/page.go Outdated Show resolved Hide resolved
common/page.go Outdated Show resolved Hide resolved
common/page.go Show resolved Hide resolved
ankur22
ankur22 previously approved these changes Aug 25, 2023
Copy link
Collaborator

@ankur22 ankur22 left a comment

Choose a reason for hiding this comment

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

This is an awesome feature! Thanks for working on it.

I left some minor comments.

My only concern is around the requirement to need to work with page.close otherwise this is great 🙂 Maybe we can think over another solution or talk it through with k6 to see if they can help.

api/console_message.go Outdated Show resolved Hide resolved
common/page.go Show resolved Hide resolved
common/page.go Outdated Show resolved Hide resolved
common/page.go Outdated Show resolved Hide resolved
common/page.go Outdated Show resolved Hide resolved
common/page.go Show resolved Hide resolved
tests/page_test.go Outdated Show resolved Hide resolved
examples/pageon.js Outdated Show resolved Hide resolved
@ka3de
Copy link
Collaborator Author

ka3de commented Aug 25, 2023

Here's a humble suggestion: Instead of this PR closing the issue, would you like to update the documentation before closing the issue?

Right, this was my plan actually @inancgumus, as there are related documentation and TS types tasks that need to be done. I will change the "closes" comment for "updates" 👍

@ka3de ka3de force-pushed the feat/835-page-on-console branch 3 times, most recently from de28297 to 1de4e41 Compare August 25, 2023 13:49
common/page.go Show resolved Hide resolved
@ka3de ka3de force-pushed the feat/835-page-on-console branch 2 times, most recently from 2548cc3 to 095f01a Compare August 28, 2023 09:10
api/console_message.go Outdated Show resolved Hide resolved
api/console_message.go Outdated Show resolved Hide resolved
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.

One suggestion about documentation and the clarifying the protocol in tests 👍

@ka3de ka3de force-pushed the feat/835-page-on-console branch 2 times, most recently from 3fb486d to adc9343 Compare August 28, 2023 12:53
Copy link
Collaborator

@ankur22 ankur22 left a comment

Choose a reason for hiding this comment

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

LGTM! This is looking great. Thanks for taking the feedback onboard.

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.

Awesome work 👏 Thanks for considering our suggestions and applying them nicely!

ka3de and others added 11 commits August 29, 2023 12:39
ConsoleMessage represents an execution of the console api.

Co-authored-by: İnanç Gümüş <[email protected]>
Stores the given event handler to be executed once the specified event
is processed. Only 'console' event is supported by now.
Subscribes page to 'consoleAPICalled' event. For each event verifies if
there is any handler registered for it through page.on('console')
method, and if there is, executes it passing the ConsoleMessage
representation for the console API method execution.
This directive is no longer necessary as these methods for both page and
frame session are used as part of page.onConsoleAPICalled method.
TaskQueue allows to queue multiple tasks to the event loop
asynchronously from one another. This is required when executing user
defined functions in the JS API that have to be executed asynchronously
whenever a certain event happens.
Events processing happens in a background goroutine from which we can
not execute the event handlers "directly" as the Goja runtime is not
thread safe. Therefore we have to use the TaskQueue in order to
synchronize the event handlers functions in the event loop from the main
thread.
Co-authored-by: ankur22 <[email protected]>
Co-authored-by: İnanç Gümüş <[email protected]>
Co-authored-by: İnanç Gümüş <[email protected]>
Adds page.on to the mapping layer so it can be accessible from JS API.
Builds a wrapper around the goja.Callable function defined by the user
in the JS API in order to minimize references to Goja from common
package, as well as to be able to use the `mapConsoleMessage` method
from the mapping layer avoiding an import cycle if it were to be called
from the `common.Page` implementation.
In this way, we only require users to close the page in order to be able
to finish the iteration when page.on() method is used in the test. If
page.on() method is not used, the iteration will end normally whether
page.close() is called or not.
@ka3de ka3de force-pushed the feat/835-page-on-console branch from adc9343 to bb4a1f6 Compare August 29, 2023 10:39
@ka3de ka3de merged commit 447c8c2 into main Aug 29, 2023
12 checks passed
@ka3de ka3de deleted the feat/835-page-on-console branch August 29, 2023 10:57
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.

3 participants