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.

Sign up for GitHub

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

Jump to bottom

Fix/jshandle evaluate #1380

Merged
merged 5 commits into from
Jun 11, 2024
Merged

Fix/jshandle evaluate #1380

merged 5 commits into from
Jun 11, 2024

Conversation

ankur22
Copy link
Collaborator

@ankur22 ankur22 commented Jun 11, 2024 •
edited
Loading

What?

This ensures that jshandle.evaluate and jshandle.evaluateHandle both set themselves as the first argument.

Why?

It's the expected behaviour which matches Playwright: https://playwright.dev/docs/api/class-jshandle#js-handle-evaluate

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)

#1379

ankur22 added 4 commits June 11, 2024 15:06
@ankur22
Fix jshandle.evaluate
4b4ef18 
Adding itself as the first argument which is the expected behaviour.
@ankur22
Fix jshandle.evaluateHandle
f1125b4 
Adding itself as the first argument which is the expected behaviour.
@ankur22
Add tests for jshandle.evaluate
3c732b3
@ankur22
Add tests for jshandle.evaluateHandle
51a12ce
@ankur22 ankur22 mentioned this pull request Jun 11, 2024
Closed
@ankur22 ankur22 requested a review from inancgumus June 11, 2024 14:09
inancgumus
inancgumus approved these changes Jun 11, 2024
View reviewed changes
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 fixing this 👍

tests/js_handle_test.go Outdated Show resolved Hide resolved
tests/js_handle_test.go Outdated Show resolved Hide resolved
tests/js_handle_test.go Outdated Show resolved Hide resolved
common/js_handle.go Show resolved Hide resolved
@ankur22
Update jshandle tests with require
b5ff202 
This will stop the test before a NPD occurs.
@ankur22 ankur22 requested a review from inancgumus June 11, 2024 14:27
@ankur22 ankur22 merged commit 4da70f2 into main Jun 11, 2024
23 checks passed
@ankur22 ankur22 deleted the fix/jshandle-evaluate branch June 11, 2024 14:46
@BrewTestBot BrewTestBot mentioned this pull request Jun 25, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@inancgumus inancgumus inancgumus approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@ankur22 @inancgumus