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

Fix some of the issues in the Poll action #778

Merged
merged 3 commits into from Apr 27, 2021
Merged

Conversation

ZekeLu
Copy link
Member

@ZekeLu ZekeLu commented Mar 25, 2021

The Poll action failed the tests very often. I have found two issues in the implementation:

  1. it does not wait for the runtime.ExecutionContextID to be available.
  2. the tests for the WithPollingMutation() option is unreliable.

This PR fixes those issues.

@kenshaw Sorry to ping you directly, but can you take some time to review this changes? The buggy implementation has already been merged into the master branch, I hope that the issues can be addressed asap. Thank you very much!

@ZekeLu ZekeLu mentioned this pull request Apr 7, 2021
most of the source code is duplicated with the Query action,
so the source code is extracted into a function.
The scenario:
1. the js in poll.html will set globalThis.__FOO to 1 with a
10ms timeout;
2. the js function pollMutation() checks the predicate at the very
first run.
When step 2 happens after step 1, pollMutation() will fulfill
without DOM mutation; which will fail the tests.
@kenshaw
Copy link
Member

kenshaw commented Apr 27, 2021

@ZekeLu appreciate the work here. A silly question, the @gmail.com address used in your commits, do you ever monitor that email? I had emailed you (personally) about a month ago, and never got a reply.

@kenshaw kenshaw merged commit b17375b into chromedp:master Apr 27, 2021
@ZekeLu
Copy link
Member Author

ZekeLu commented Apr 27, 2021

@kenshaw Oh, I don't check the inbox for a long time. gmail is blocked here 😢 so I don't check it frequently. I'm going to have a check now. Sorry!


update: Just read through your email. I'm so excited to hear from you! Thank you!

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

2 participants