Fix navigation when errorText is not empty #4862

ankur22 · 2025-06-18T10:19:36Z

What?

We now check whether errorText is not empty during a navigation, to fail the test early.

Why?

When errorText is not empty and err is nil, it means that the navigation failed; for example when a DNS lookup fails.

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)

#4861

ankur22 · 2025-06-18T10:36:02Z

internal/js/modules/k6/browser/common/frame_session.go

+	if errorText != "" {
+		err = fmt.Errorf("navigation failed: %v", errorText)
+	}


In k6 browser, when a navigation fails due to something like a DNS lookup, at the moment it continues on with the navigation and seems to assume that it succeeded. This is a problem down the line since future calls to APIs in the test script will eventually fail.

Running all the browser tests, i realised it seems that the old behaviour is valid? For example TestBlockHostname now fails on page.goto, but the test expects the page.goto call to succeed. What do you think the behaviour should be?

CC @inancgumus

Can you help me understand your concerns? How can I run and compare the "new" and "old" behavior? Do you mean the code in this PR by the "new" behavior? Thanks!

internal/js/modules/k6/browser/tests/page_test.go

codebien · 2025-06-18T12:52:33Z

internal/js/modules/k6/browser/common/frame_session.go

@@ -629,7 +629,7 @@ func (fs *FrameSession) navigateFrame(frame *Frame, url, referrer string) (strin
 			err = fmt.Errorf("%q: %w", errorText, err)
 		}
 	}
-	return documentID.String(), err
+	return documentID.String(), errorText, err


Suggested change

return documentID.String(), errorText, err

return documentID.String(), err

Why do we need to return this error? Can't we throw an error here if we identify that the errorText is not empty?

In addition, a DNS error is generally an error on any system, why isn't the case here? Are we sure to not silencing the error somewhere else?

Right, i agree that this is a smell. Happy to have any suggestions on how to improve this.

Context

The issue in this case where err is nil and errorText is not empty is that once we return an error after calling navigateFrame, it will return an error back to the caller, but the event loop will contain older events which could be consumed by the next event handler of the same type.

k6 browser uses event loops in the background. We register an event handler here to listen out for incoming EventFrameNavigation CDP events from chromium once a page navigation occurs -- it turns out that this event is fired even when a DNS issue occurs, which makes sense since if you type in https://blah.madeupurl.com into chromium you will see This site can’t be reached page, so a navigation does indeed occur.

If we don't consume the event then the next time we create the same event handler it might consume the older event, which is what was happening in TestBlockHostnames when i ran the test when returning err early.

After an event is consumed the event handler is destroyed regardless of whether it successfully handled the event or not.

To avoid this, the handling of errorText needs to be done differently to err when calling navigateFrame (code link). The error case is handled straight away since we assume that we will never get a EventFrameNavigation event. For the case where errorText is not empty, we need to wait for the EventFrameNavigation, and then we can return an error (code link).

Changing the event loop itself will be super complex -- we have tried to make fixes and refactors in the past and it's super difficult which can cause other issues further down the line! I want to avoid that.

Have you seen #4918? It seems to be related 🤔

codebien · 2025-06-18T15:39:30Z

@ankur22 the latest added tests seems to fail

ankur22 · 2025-06-20T13:09:32Z

@ankur22 the latest added tests seems to fail

🤷 it's one of those cases where it works on my machine and not in CI. I've tried working with GOMAXPROCS and i can't get the test to fail locally. I am seeing an unusaly high number of CI failures this week, could it be a change in githubs actions?

@inancgumus any pointers on how I could try to cox the new TestClickSkipRequestForBlobFile and TestURLSkipRequestForBlobFile to fail locally?

codebien · 2025-06-20T13:50:45Z

it's one of those cases where it works on my machine and not in CI.

@ankur22 Did you try to run with -race enabled?

could it be a change in githubs actions?

I don't think so, we should see it across all the modules. Instead, they seem localized on the browser.

Honestly, based on all the Please call browser.close only once reported, it smells like there is a data race somewhere.

ankur22 · 2025-06-20T13:56:28Z

@ankur22 Did you try to run with -race enabled?

yeah i used that too.

I don't think so, we should see it across all the modules. Instead, they seem localized on the browser.

The browser module is a little more sensitive to github action changes since the tests require significantly more resource than the other modules to run a single test with a browser.

Honestly, based on all the Please call browser.close only once reported, it smells like there is a data race somewhere.

That's nothing new (unfortunately), neither are the context canceled issues which end the test early.

inancgumus · 2025-06-24T20:31:52Z

could it be a change in githubs actions

@ankur22 Do you mean the current (this PR's) failures? The Github actions should not affect the outcome of these runs unless you see a Vault communication error.

@inancgumus any pointers on how I could try to cox the new TestClickSkipRequestForBlobFile and TestURLSkipRequestForBlobFile to fail locally?

It's probably a Windows issue because the preceding errors (before your tests) are often like this:

time="2025-06-24T16:38:47Z" level=error msg="process with PID 4716 unexpectedly ended: exit status 1" category=browser elapsed="0 ms" source=browser
time="2025-06-24T16:38:47Z" level=error msg="cleaning up the user data directory: unlinkat C:\\Users\\RUNNER~1\\AppData\\Local\\Temp\\k6browser-data-2454286471\\Default\\Account Web Data: The process cannot access the file because it is being used by another process." category="Browser:Close" elapsed="42 ms" source=browser
time="2025-06-24T16:38:52Z" level=error msg="error on evaluating window.k6SpanId: evaluating JS in global context: context canceled" category="FrameSession:onFrameNavigated" elapsed="0 ms" source=browser

I can get the same test error when I give it a file other than blob.html, like foo.html.

* Base release notes for v1.1.0 * Add release notes for dependabot PRs * Add #4809 and #4831 into the v1.1.0 release note * Add #4845 to the v1.1.0 release notes * Add example for locator.count * Remove await on expect in count example * Add section for nth, first and count in v1.1.0... ... release notes. * Update examples to work with latest test library This also updates the nth, first and last example. * Add some PRs from the beginning of the cycle into release notes * Add external PRs, among others, to release notes * Clean up stuff from release notes template * Add remaining PRs into release notes * Add #4862 to the v1.1.0 release note * Apply suggestions from code review Co-authored-by: Mihail Stoykov <312246+mstoykov@users.noreply.github.com> * Add #4864 to v1.1.0 release notes * Remove #4862 from v1.1.0 * Apply suggestions from code review Co-authored-by: İnanç Gümüş <inanc.gumus@grafana.com> * web/crypto updates * Update release notes/v1.1.0.md Co-authored-by: Mihail Stoykov <312246+mstoykov@users.noreply.github.com> * docs: document v1.1.0 forward roadmap * Refine experimental modules notes --------- Co-authored-by: ankur22 <ankur.agarwal@grafana.com> Co-authored-by: Mihail Stoykov <312246+mstoykov@users.noreply.github.com> Co-authored-by: İnanç Gümüş <inanc.gumus@grafana.com> Co-authored-by: oleiade <theo@crevon.me> Co-authored-by: Ivan <2103732+codebien@users.noreply.github.com>

This breaks the test into three separate tests so that we can properly focus on each different area of skipping loading of a file/blob.

Co-authored-by: Ivan <2103732+codebien@users.noreply.github.com>

ankur22 · 2025-07-14T10:43:09Z

Closing this PR in favour of #4918

ankur22 requested a review from a team as a code owner June 18, 2025 10:19

ankur22 requested review from codebien and joanlopez and removed request for a team June 18, 2025 10:19

ankur22 added this to the v1.1.0 milestone Jun 18, 2025

ankur22 added a commit that referenced this pull request Jun 18, 2025

Add #4862 to the v1.1.0 release note

eef3e9e

ankur22 requested a review from inancgumus June 18, 2025 10:34

ankur22 commented Jun 18, 2025

View reviewed changes

ankur22 force-pushed the fix/text-error-navigation branch 2 times, most recently from 08e8f07 to 4e02ef8 Compare June 18, 2025 11:54

codebien reviewed Jun 18, 2025

View reviewed changes

ankur22 force-pushed the fix/text-error-navigation branch 3 times, most recently from db7b358 to b9f1d7f Compare June 20, 2025 13:07

ankur22 force-pushed the fix/text-error-navigation branch from b9f1d7f to ac8669b Compare June 24, 2025 16:29

ankur22 modified the milestones: v1.1.0, v1.2.0 Jun 25, 2025

ankur22 added a commit that referenced this pull request Jun 25, 2025

Remove #4862 from v1.1.0

8b242c3

ankur22 mentioned this pull request Jul 8, 2025

Chromium in docker times out unexpectedly on various API call #4902

Open

ankur22 and others added 5 commits July 10, 2025 23:27

Fix navigation when textError is not empty

e9f2b6e

Add test for when errorText is not empty

6429b62

Refactor the TestURLSkipRequest test

1642d23

This breaks the test into three separate tests so that we can properly focus on each different area of skipping loading of a file/blob.

Fix the tests after goto now returns an error

176de1b

Update internal/js/modules/k6/browser/tests/page_test.go

86e3a6d

Co-authored-by: Ivan <2103732+codebien@users.noreply.github.com>

ankur22 force-pushed the fix/text-error-navigation branch from ac8669b to 86e3a6d Compare July 10, 2025 22:27

ankur22 closed this Jul 14, 2025

mstoykov removed this from the v1.2.0 milestone Aug 4, 2025

ankur22 deleted the fix/text-error-navigation branch August 8, 2025 16:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Fix navigation when errorText is not empty #4862

Fix navigation when errorText is not empty #4862

Uh oh!

ankur22 commented Jun 18, 2025 •

edited

Loading

Uh oh!

ankur22 Jun 18, 2025

Uh oh!

inancgumus Jun 24, 2025 •

edited

Loading

Uh oh!

Uh oh!

codebien Jun 18, 2025

Uh oh!

ankur22 Jun 18, 2025

Uh oh!

joanlopez Jul 14, 2025

Uh oh!

codebien commented Jun 18, 2025

Uh oh!

ankur22 commented Jun 20, 2025

Uh oh!

codebien commented Jun 20, 2025 •

edited

Loading

Uh oh!

ankur22 commented Jun 20, 2025 •

edited

Loading

Uh oh!

inancgumus commented Jun 24, 2025 •

edited

Loading

Uh oh!

ankur22 commented Jul 14, 2025

Uh oh!

Uh oh!

	return documentID.String(), errorText, err
	return documentID.String(), err

Fix navigation when errorText is not empty #4862

Fix navigation when errorText is not empty #4862

Uh oh!

Conversation

ankur22 commented Jun 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What?

Why?

Checklist

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

Related PR(s)/Issue(s)

Uh oh!

ankur22 Jun 18, 2025

Choose a reason for hiding this comment

Uh oh!

inancgumus Jun 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

codebien Jun 18, 2025

Choose a reason for hiding this comment

Uh oh!

ankur22 Jun 18, 2025

Choose a reason for hiding this comment

Context

Uh oh!

joanlopez Jul 14, 2025

Choose a reason for hiding this comment

Uh oh!

codebien commented Jun 18, 2025

Uh oh!

ankur22 commented Jun 20, 2025

Uh oh!

codebien commented Jun 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ankur22 commented Jun 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

inancgumus commented Jun 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ankur22 commented Jul 14, 2025

Uh oh!

Uh oh!

ankur22 commented Jun 18, 2025 •

edited

Loading

inancgumus Jun 24, 2025 •

edited

Loading

codebien commented Jun 20, 2025 •

edited

Loading

ankur22 commented Jun 20, 2025 •

edited

Loading

inancgumus commented Jun 24, 2025 •

edited

Loading