Skip to content
This repository has been archived by the owner on Jan 30, 2025. It is now read-only.

Fix interacting with an element inside nested IFrames #1096

Merged
merged 4 commits into from
Nov 29, 2023

Conversation

bandorko
Copy link
Contributor

@bandorko bandorko commented Nov 9, 2023

What?

Page.getFrameElement now finds the root frame. Only the root frame has a FrameSession.

Why?

With k6 v0.47.0 github.com/grafana/xk6-browser@v1.1.0/common/page.go:274 panicked because the parent session was nil.

Checklist

  • I have performed a self-review of my code
  • I have added tests for my changes
  • I have commented on my code, particularly in hard-to-understand areas

Related PR(s)/Issue(s)

Closes: #1087

@CLAassistant
Copy link

CLAassistant commented Nov 9, 2023

CLA assistant check
All committers have signed the CLA.

@ahlstro
Copy link

ahlstro commented Nov 27, 2023

We would very much like this fix to be merged in the next release because as of now our test-script for our customer's behalf cannot run fully to the end. Thank you kindly.

Copy link
Collaborator

@ankur22 ankur22 left a comment

Choose a reason for hiding this comment

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

Thanks for the fix! LGTM.

Just one request, which is to tidy up the test.

EDIT: Can you please squash the two commits which are related to the test.

tests/page_test.go Show resolved Hide resolved
@ankur22 ankur22 force-pushed the fix/1087-getFrameElement branch from e18e748 to a86bd4b Compare November 28, 2023 13:21
tests/page_test.go Outdated Show resolved Hide resolved
@bandorko bandorko force-pushed the fix/1087-getFrameElement branch from a86bd4b to 7ca6bde Compare November 28, 2023 23:48
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.

LGTM 👍 Thanks for this 🙇 Can you also rename the first commit to: Fix getFrameElement to find root frame? One more suggestion below.

tests/page_test.go Outdated Show resolved Hide resolved
@inancgumus inancgumus changed the title fix getFrameElement Fix getFrameElement for nested IFrames Nov 29, 2023
@inancgumus inancgumus changed the title Fix getFrameElement for nested IFrames Fix interacting with an element inside nested IFrames Nov 29, 2023
tests/page_test.go Outdated Show resolved Hide resolved
Copy link
Collaborator

@ankur22 ankur22 left a comment

Choose a reason for hiding this comment

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

Great work! 🎉

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.

Thanks for this 🙇 Please remember to rename the first commit to: Fix getFrameElement to find root frame.

@ankur22 ankur22 force-pushed the fix/1087-getFrameElement branch from 2438d02 to 4eef973 Compare November 29, 2023 12:21
@ankur22
Copy link
Collaborator

ankur22 commented Nov 29, 2023

@bandorko I've amended the commit message that @inancgumus was suggesting. I'll be merging this change in once the CI pipeline passes.

@ankur22 ankur22 merged commit d785975 into grafana:main Nov 29, 2023
13 checks passed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Interacting with an element inside nested IFrames
5 participants