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

#99 Add ability to connect to an already existing instance of Chrome #100

Merged
merged 5 commits into from
Sep 23, 2018
Merged

#99 Add ability to connect to an already existing instance of Chrome #100

merged 5 commits into from
Sep 23, 2018

Conversation

gidztech
Copy link
Contributor

See #99 for discussion.

gregberge
gregberge requested changes Aug 15, 2018
View reviewed changes
Copy link
Member

@gregberge gregberge left a comment

Choose a reason for hiding this comment

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

I think adding a connect section to configuration is the good way to do. Just some problems:

  • Code can be simplified
  • Async configuration should not be part of this PR

packages/jest-environment-puppeteer/src/readConfig.js Outdated
@@ -38,7 +38,13 @@ async function readConfig() {
}

// eslint-disable-next-line global-require, import/no-dynamic-require
const localConfig = require(absConfigPath)
const requiredConfig = require(absConfigPath)
Copy link
Member

Choose a reason for hiding this comment

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

Why should we support async config? I think it is another issue.

@gidztech
Copy link
Contributor Author

gidztech commented Aug 16, 2018 •
edited
Loading

The async function config I originally added was so that I could run an async task first (e.g. launching Chrome and get the WebSocket URI) before returning the connect object with the URI.

However, I realised I could do that async work in a Jest globalSetup file, which executes beforehand. I can write the WebSocket URI to disk and read it synchronously in jest-puppeteer.config.js. Therefore, adding an async function is no longer needed right now.

I will update the PR this weekend.

@gregberge
Copy link
Member

@gidztech OK thanks

@gidztech gidztech force-pushed the connect branch 2 times, most recently from c1ee05e to 2ff4bdf Compare August 19, 2018 20:45
@gidztech
Copy link
Contributor Author

@neoziro I made some simplifications thanks to my forgotten friend "object spread"

@gidztech
Copy link
Contributor Author

gidztech commented Sep 3, 2018 •
edited
Loading

@neoziro Let me know if there's anything else to do. It would be good to get this in.

@gregberge
Copy link
Member

@gidztech good for me! Can you update readme please? 🙏

@gidztech
Copy link
Contributor Author

gidztech commented Sep 9, 2018

@neoziro Added documentation (with example usage)

@gregberge gregberge merged commit 3fcbaf8 into argos-ci:master Sep 23, 2018
@gregberge gregberge mentioned this pull request Sep 23, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gregberge gregberge gregberge 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
@gidztech @gregberge