Skip to content

Conversation

waisbrot
Copy link
Contributor

@waisbrot waisbrot commented Oct 7, 2025

Why

window might be undefined (inside a chrome extension context, for example), in which case isK6BrowserSession crashes instead of just returning false.

What

Check for undefined before dereferencing window.

Links

fixes #1644

Checklist

  • Tests added
  • Changelog updated
  • Documentation updated

@CLAassistant
Copy link

CLAassistant commented Oct 7, 2025

CLA assistant check
All committers have signed the CLA.

@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@codecapitano codecapitano changed the title Don't assume a window when testing for k6 fix: don't assume a window when testing for k6 Oct 9, 2025
Copy link
Collaborator

@codecapitano codecapitano left a comment

Choose a reason for hiding this comment

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

Good catch. Many thanks for the fix 🙏

@waisbrot
Copy link
Contributor Author

waisbrot commented Oct 9, 2025

I'm not sure how to write a test for this and IMO it doesn't need a documentation update. Please LMK if you'd like either of those added.

@codecapitano
Copy link
Collaborator

I'm not sure how to write a test for this and IMO it doesn't need a documentation update. Please LMK if you'd like either of those added.

Hey Nathaniel 👋

It doesn't need a doc update.

I think for testing you can look at this test case.
Maybe something like that but not adding the k6 object to global and asserting with expect(...).toThrow(error?) works?

Would be great to have the test but in this case i think we can be pragmatic so it's not mandatory.
Maybe you can try but if it would take to long in contrast to the value it adds let's skip it.

@waisbrot
Copy link
Contributor Author

Thanks for the pointer. I took a quick look, but I'm not going to have time to work on tests for a while, I think. So let's say skip.

Copy link
Collaborator

@eskirk eskirk left a comment

Choose a reason for hiding this comment

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

great fix - thank you

`window` might be `undefined` (inside a chrome extension context, for
example), in which case `isK6BrowserSession` crashes instead of just
returning `false`.

Fix by checking for `undefined` before dereferencing it.
@codecapitano codecapitano merged commit b638440 into grafana:main Oct 20, 2025
12 checks passed
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.

4 participants