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

Allow listening for remote automation events #17595

Closed
wants to merge 1 commit into from

Conversation

Andarist
Copy link
Contributor

@Andarist Andarist commented Aug 4, 2021

User facing changelog

Cypress.automation accepts a new remote:debugger:event' command. This allows you to listen for remote automation events like Page.javascriptDialogOpening.

Additional details

I'd like to test stuff like native browser dialogs. It's impossible without being able to hook into an event like Page.javascriptDialogOpening

How has the user experience changed?

This introduces a new capability for advanced users.

PR Tasks

If this PR gets green light from your team then I could work on the remaining tasks.

@Andarist Andarist requested a review from a team as a code owner August 4, 2021 09:57
@Andarist Andarist requested review from flotwig and jennifer-shehane and removed request for a team August 4, 2021 09:57
@cypress-bot
Copy link
Contributor

cypress-bot bot commented Aug 4, 2021

Thanks for taking the time to open a PR!

@Andarist
Copy link
Contributor Author

Andarist commented Aug 5, 2021

@jennifer-shehane I see that you have assigned me to this PR - does this mean that I should work on the remaining items from the list?

Copy link
Contributor

@flotwig flotwig left a comment

Choose a reason for hiding this comment

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

remote:debugger:protocol was added for internal testing, but since we actually have consumers of these APIs, can you add some tests for remote:debugger:event and remote:debugger:protocol please? You could add them as server unit tests in packages/server/test/unit/browsers/cdp_automation_spec.ts.

Comment on lines +272 to +273
case 'remote:debugger:event':
return this.onFn(data.event, data.callback)
Copy link
Contributor

Choose a reason for hiding this comment

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

I understand why you want to add this. For additional context, we are planning to add official CDP support through a nicer API in the future, see this comment for some ideas.

One part that is TBD is how the planned API will handle selecting CDP Targets. To make your change forward-compatible with the future feature, can you add a targetId to the callback to indicate that this event is originating from Cypress's main CDP Target? Like, instead of calling data.callback with the raw eventData, call it with { targetId: 'main', eventData }, or something like that.

That way we can add CDP Target selection in a future PR that is forward-compatible with this change (for #7942). For the Cypress-owned Target, targetId could be main, for all others it could be the targetId returned by Target.createTarget.

@jennifer-shehane
Copy link
Member

Unfortunately we have to close this PR due to inactivity. Please open a new PR addressing the original issue and any requested changes.

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.

None yet

3 participants