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

Merged
merged 12 commits into from
Feb 19, 2025
Merged

Add page.on('request') #4290

merged 12 commits into from
Feb 19, 2025

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)

@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
@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.

@ankur22 ankur22 force-pushed the add/page-on-request branch 3 times, most recently 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.

inancgumus
inancgumus previously approved these changes Jan 30, 2025
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)

@codebien
Copy link
Contributor

@inancgumus can you rebase and resolve the conflicts here, please? 🙏

@inancgumus inancgumus force-pushed the add/page-on-request branch 2 times, most recently from 6b8b7f9 to 6f26ce7 Compare February 14, 2025 14:59
@inancgumus inancgumus requested a review from codebien February 14, 2025 15:27
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').
The issue was that HeadersArray was out of order. If they're put in
order then the comparison can be made.
@inancgumus inancgumus merged commit 4a6887a into master Feb 19, 2025
28 checks passed
@inancgumus inancgumus deleted the add/page-on-request branch February 19, 2025 15:47
@inancgumus inancgumus added this to the v1.0.0-rc1 milestone Feb 19, 2025
@inancgumus inancgumus added the documentation-needed A PR which will need a separate PR for documentation label Feb 19, 2025
@inancgumus inancgumus removed the documentation-needed A PR which will need a separate PR for documentation label Feb 20, 2025
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