Skip to content

browser: add page.on('requestfailed') event handler - #5481

Merged
ankur22 merged 10 commits into
grafana:masterfrom
pkalsi97:feat/4299
Jan 16, 2026
Merged

browser: add page.on('requestfailed') event handler#5481
ankur22 merged 10 commits into
grafana:masterfrom
pkalsi97:feat/4299

Conversation

@pkalsi97

@pkalsi97 pkalsi97 commented Dec 11, 2025

Copy link
Copy Markdown
Contributor

What?

This PR Adds the page.on('requestfailed') event handler to the k6 browser module, following the Playwright API.

This event fires when a network request fails to reach the server:
in the following cases : DNS errors, connection refused, timeouts, etc.
It does not fire for HTTP 4xx/5xx responses, which are successful network requests.

Note: I have closely followed the implementation of page.on('request') and page.on('response') events.

Why?

Reasons mentioned in the issue #4299 description.

Checklist

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

Checklist: Documentation (only for k6 maintainers and if relevant)

Please do not merge this PR until the following items are filled out.

  • I have added the correct milestone and labels to the PR.
  • I have updated the release notes: link
  • I have updated or added an issue to the k6-documentation: grafana/k6-docs#NUMBER if applicable
  • I have updated or added an issue to the TypeScript definitions: grafana/k6-DefinitelyTyped#NUMBER if applicable

Related PR(s)/Issue(s)

Closes: #4299
Parent: #4232

@pkalsi97
pkalsi97 requested a review from a team as a code owner December 11, 2025 08:38
@pkalsi97
pkalsi97 requested review from codebien and mstoykov and removed request for a team December 11, 2025 08:38
@pkalsi97
pkalsi97 temporarily deployed to azure-trusted-signing December 11, 2025 08:56 — with GitHub Actions Inactive
@pkalsi97
pkalsi97 temporarily deployed to azure-trusted-signing December 11, 2025 08:58 — with GitHub Actions Inactive
@pkalsi97 pkalsi97 changed the title browser: add page.on('requestfailed') event browser: add page.on('requestfailed') event handler Dec 11, 2025
@pkalsi97
pkalsi97 temporarily deployed to azure-trusted-signing December 12, 2025 08:20 — with GitHub Actions Inactive
@pkalsi97
pkalsi97 temporarily deployed to azure-trusted-signing December 12, 2025 08:22 — with GitHub Actions Inactive

@mstoykov mstoykov left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thank you for the PR, and sorry for the slow reviews - the holidays and catching up has been getting in the way.

Over all looks really good, my only concern is the skipping of the test in the middle. I really do not want this to be happening, either we know that this hsouldn't work in some cases and we skip early or we are running the test. This test currently will practically never fail - it will just be skipped. Which isn't good for tests.

Comment thread internal/js/modules/k6/browser/tests/page_test.go Outdated
@pkalsi97

Copy link
Copy Markdown
Contributor Author

@mstoykov Thanks a lot for taking a look at it, i hope you had a great holiday.

I have updated the test to follow a more deterministic approach where:

  1. page.Route() with route.Abort() is used to intercept and fail requests at the browser level, instead of relying on external network behavior.
  2. Removes the conditional t.Skip(), test will now fail if no requestfailed events are captured

@pkalsi97
pkalsi97 requested a review from mstoykov January 13, 2026 07:08
Comment thread internal/js/modules/k6/browser/tests/page_test.go Outdated
@pkalsi97
pkalsi97 requested a review from mstoykov January 13, 2026 11:57
mstoykov
mstoykov previously approved these changes Jan 13, 2026

@mstoykov mstoykov left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM now

@ankur22
ankur22 self-requested a review January 15, 2026 14:15
ankur22
ankur22 previously approved these changes Jan 15, 2026

@ankur22 ankur22 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Awesome! Thanks for this 🚀

@ankur22

ankur22 commented Jan 15, 2026

Copy link
Copy Markdown
Contributor

@pkalsi97 I've merged in your other PR (#5486) and now we're seeing two things that need some attention:

  1. lint issue
  2. Merge conflict

Could you please take a look at these two and resolve them?

@pkalsi97
pkalsi97 temporarily deployed to azure-trusted-signing January 15, 2026 15:46 — with GitHub Actions Inactive
@pkalsi97
pkalsi97 temporarily deployed to azure-trusted-signing January 15, 2026 15:49 — with GitHub Actions Inactive
@pkalsi97
pkalsi97 dismissed stale reviews from ankur22 and mstoykov via c25f560 January 15, 2026 17:10
@pkalsi97

Copy link
Copy Markdown
Contributor Author

@ankur22 Thanks i have addressed the lint issue and resolved the conflict. :)

@ankur22
ankur22 merged commit 4a9c127 into grafana:master Jan 16, 2026
39 of 41 checks passed
@ankur22 ankur22 added this to the v1.6.0 milestone Jan 16, 2026
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.

Implement page.on('requestfailed')

4 participants