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

Simplify lifecycle events for frames #593

Closed
1 task done
ankur22 opened this issue Oct 17, 2022 · 3 comments · Fixed by #644 or #647
Closed
1 task done

Simplify lifecycle events for frames #593

ankur22 opened this issue Oct 17, 2022 · 3 comments · Fixed by #644 or #647
Assignees
Labels
events CDP or internal events related. refactor
Milestone

Comments

@ankur22
Copy link
Collaborator

ankur22 commented Oct 17, 2022

This issue was opened after some discussions in #578.

Basically, the issue was that a test would time out when xk6-browser navigates to google.com and waits for networkIdle. The issue was that we weren't correctly handling the CDP event from the browser.

While working on the fix, it became clear that the lifecycle event code was too complex for what we needed and it might be worth removing.

#593 (comment)

@ankur22 ankur22 self-assigned this Oct 17, 2022
@ankur22 ankur22 added this to the v0.6.0 milestone Oct 17, 2022
@inancgumus inancgumus added the events CDP or internal events related. label Oct 18, 2022
@inancgumus
Copy link
Member

inancgumus commented Oct 18, 2022

The issue was that we weren't correctly handling the CDP event from the browser.

While working on the fix, it became clear that the lifecycle event code was too complex for what we needed and it might be worth removing.

I believe the issue should not be about removing stuff. Instead, finding ways to understand why the existing constructs exist in the first place. For example, in #578, it turned out that the actual problem was about not handling the removal events.

@ankur22
Copy link
Collaborator Author

ankur22 commented Oct 18, 2022

Research: How do Playwright and Puppeteer do it. Maybe add a Mermaid diagram.

I don't think looking at puppeteer and playwright should be how we determine what's best for xk6-browser. They're a good source of information on why things have been done a certain way in xk6-browser though.

I believe the issue should not be about removing stuff. Instead, #578 (comment) why the existing constructs exist in the first place. For example, in #578, #578 (comment) that the actual problem was about not handling the removal events.

I agree that a suite of tests is what we need now, and then we can refactor the code, whatever the solution maybe as long as the tests pass.

@inancgumus inancgumus modified the milestones: v0.6.0, v0.7.0 Nov 8, 2022
ankur22 added a commit that referenced this issue Nov 8, 2022
There's no need for recalculateLifecycle since all it is now doing is
emitting the event to internal handlers. So refactor/move it to
onLifecycleEvent.

Closes: #593
ankur22 added a commit that referenced this issue Nov 8, 2022
There's no need for recalculateLifecycle since all it is now doing is
emitting the event to internal handlers. So refactor/move it to
onLifecycleEvent.

Closes: #593
ankur22 added a commit that referenced this issue Nov 11, 2022
There's no need for recalculateLifecycle since all it is now doing is
emitting the event to internal handlers. So refactor/move it to
onLifecycleEvent.

Closes: #593
@inancgumus inancgumus linked a pull request Dec 5, 2022 that will close this issue
@inancgumus
Copy link
Member

inancgumus commented Dec 5, 2022

I've removed PR #647 from the project board and linked it to this issue. This way, we can reduce the number of issues and can easily track their implementation and tests from a single place. IMO, the project items should mostly be about public-facing issues rather than PRs (not always possible).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
events CDP or internal events related. refactor
Projects
None yet
2 participants