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('request') #4290

Open
wants to merge 12 commits into
base: master
Choose a base branch
from
Open

Add page.on('request') #4290

wants to merge 12 commits into from

Conversation

ankur22
Copy link
Contributor

@ankur22 ankur22 commented Jan 28, 2025

What?

This implements the page.on('request') API from Playwright. When the handler is created, all subsequent requests will be directed to the handler, where the user can interact with the read only request. The request is intercepted just before it is sent from the browser to the website under test.

An example usage is:

import { browser } from 'k6/browser'

export const options = {
  scenarios: {
    ui: {
      executor: 'shared-iterations',
      options: {
        browser: {
          type: 'chromium',
        },
      },
    },
  },
}

export default async function () {
  const page = await browser.newPage()

  // Logs the url of the request
  page.on('request', async (request) => {
    console.log(request.url());
  })

  await page.goto('https://quickpizza.grafana.com/', { waitUntil: 'networkidle' })

  await page.close();
}

Why?

This can be useful to:

  • retrieve data that is to be used later;
  • assert certain conditions on an expected request;

Checklist

  • I have performed a self-review of my code.
  • I have added tests for my changes.
  • I have run linter locally (make lint) and all checks pass.
  • I have run tests locally (make tests) and all tests pass.
  • I have commented on my code, particularly in hard-to-understand areas.

Related PR(s)/Issue(s)

Updates: #4281

@ankur22 ankur22 requested a review from a team as a code owner January 28, 2025 10:42
@ankur22 ankur22 requested review from inancgumus and codebien and removed request for a team January 28, 2025 10:42
@ankur22 ankur22 marked this pull request as draft January 28, 2025 10:42
@ankur22 ankur22 force-pushed the add/page-on-request branch 2 times, most recently from bd7b987 to 05b8969 Compare January 28, 2025 13:50
@ankur22 ankur22 marked this pull request as ready for review January 29, 2025 16:34
This is where the request will be set for the page.on handler to read.
This method is to be called when a new request is about to be sent
from chrome to the WuT. It takes the request and send it to the page.on
handler where the user test script can read the request data from.
This is a temporary fix.

When working with page.on('request') the timing values haven't yet been
received, which are part of the response object. This will need to be
fixed later when we can wait for a response while working with
page.on('request').
@ankur22 ankur22 force-pushed the add/page-on-request branch from 05b8969 to 6ac9bd0 Compare January 29, 2025 16:34
@ankur22 ankur22 changed the title Add page.on('request') Add page.on('request') Jan 29, 2025
@ankur22 ankur22 mentioned this pull request Jan 29, 2025
5 tasks
@ankur22 ankur22 force-pushed the add/page-on-request branch from 6ac9bd0 to 09fbe5d Compare January 29, 2025 19:46
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.

Nice bit of work 👏 Some suggestions.

internal/js/modules/k6/browser/tests/page_test.go Outdated Show resolved Hide resolved
internal/js/modules/k6/browser/tests/page_test.go Outdated Show resolved Hide resolved
internal/js/modules/k6/browser/tests/page_test.go Outdated Show resolved Hide resolved
internal/js/modules/k6/browser/tests/page_test.go Outdated Show resolved Hide resolved
@ankur22 ankur22 force-pushed the add/page-on-request branch 2 times, most recently from 7d131b6 to 2709a6c Compare January 30, 2025 09:40
@ankur22 ankur22 force-pushed the add/page-on-request branch from 2709a6c to 522a411 Compare January 30, 2025 09:41
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 more non-critical thing I noticed.

internal/js/modules/k6/browser/tests/page_test.go Outdated Show resolved Hide resolved
inancgumus
inancgumus previously approved these changes Jan 30, 2025
The issue was that HeadersArray was out of order. If they're put in
order then the comparison can be made.
Comment on lines +502 to +503
p.eventHandlersMu.RUnlock()
defer p.eventHandlersMu.RLock()
Copy link
Contributor

@codebien codebien Jan 31, 2025

Choose a reason for hiding this comment

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

Why do we need to lock again? Isn't the lock before already safe-guarding for it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, this looks a bit odd. The reason behind this is to allow the handler to be able to add more handlers. This is a behaviour that Playwright exhibits and also documents, so it's something we're replicating.

page.on('request', async () => {
    page.on('response', async () => {
        // Do something with the request and response data...
    })
})

Copy link
Member

Choose a reason for hiding this comment

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

@codebien I asked the same question before for another related page.on method. Here's a little bit more detail @ankur22 previously answered: grafana/xk6-browser#1456 (comment)

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