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

feat(launcher): add new launcher option waitForInitialPage #7105

Merged
merged 7 commits into from May 6, 2021

Conversation

starrify
Copy link
Contributor

The existing behavior is expected to be unchanged as the value defaults to true.
Adding such option would allow user to skip the initial wait.

Relevant to #3630

The existing behavior is expected to be unchanged as the value defaults to `true`.
Adding such option would allow user to skip the initial wait.
@google-cla google-cla bot added the cla: yes label Apr 19, 2021
@starrify
Copy link
Contributor Author

Hi @jschfflr @OrKoN :) Sorry for bothering you. I picked your names by randomly checking a few recently merged PRs and looking for the reviewer's name.

I understand that your team may have been rather busy working on other high priority tasks. However given that I'm rather new to contributing to this project, I'm yet unsure whether there's any other necessary step for me to do before expecting a review.

Could you kindly suggest whether there is something else that I should have done, or shall I just keep waiting patiently? Thanks! :)

@jschfflr
Copy link
Contributor

Hi @starrify, your change looks good to me! Thanks for contributing to puppeteer.
@OrKoN Do you think we need a test for this?

@OrKoN
Copy link
Collaborator

OrKoN commented Apr 28, 2021

Could we add a test for launching with this parameter and --no-startup-window to https://github.com/puppeteer/puppeteer/blob/main/test/launcher.spec.ts?

@starrify
Copy link
Contributor Author

Thanks a lot @jschfflr @OrKoN for your time and opinions! :)
I have added a new commit 75e5377 to this PR to introduce a new test case for this option.

@starrify
Copy link
Contributor Author

Sorry, I forgot to run the linter for the added test case, and it turns out that the long test subject line eventually triggers linter errors.

I have pushed a new commit 11b6d12 to resolve that.

@OrKoN
Copy link
Collaborator

OrKoN commented Apr 29, 2021

The failure seems to be unrelated to this change. I think it's being looked at separately atm.

@OrKoN
Copy link
Collaborator

OrKoN commented May 6, 2021

starrify@ could you please rebase?

@starrify
Copy link
Contributor Author

starrify commented May 6, 2021

Sorry I wasn't aware of the conflict.. I've just updated the branch to have the conflicts resolved.
Please let me know if you'd like to have the commit history further cleaned, in which case I may squash the changes and do a force push to leave only one commit in the PR.

@OrKoN OrKoN enabled auto-merge (squash) May 6, 2021 20:15
@OrKoN
Copy link
Collaborator

OrKoN commented May 6, 2021

@starrify thanks! It looks fine. I have enabled auto-merge (and we squash all PRs by default) and hopefully it gets merged once CI is done (we had some flaky Firefox tests recently so it might fail again).

@OrKoN OrKoN merged commit 2605309 into puppeteer:main May 6, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants