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

Enable passing of a custom userDataDir to launcher #2357

Merged
merged 4 commits into from
Jun 5, 2017
Merged

Enable passing of a custom userDataDir to launcher #2357

merged 4 commits into from
Jun 5, 2017

Conversation

samccone
Copy link
Contributor

@samccone samccone commented May 25, 2017 •
edited
Loading

  • rename TMP_DIR to a more accurate name. Since this is a private
    internal value, this is non-breaking.

Fixes #2291
Ref #2092

@samccone samccone requested a review from paulirish May 25, 2017 02:44
@samccone
Copy link
Contributor Author

CI failure is just due to flake.

@paulirish
Copy link
Member

✅ Confirmed it works via adding a userDataDir option into the https://github.com/GoogleChrome/lighthouse/blob/master/docs/readme.md#using-programmatically example.

doesn't delete if passed a custom path. clears the profile if we generated.

Nice!

@paulirish
Copy link
Member

Tests? wdyt

@wardpeet
Copy link
Collaborator

would be nice to test this on appveyor 😄

@samccone
Copy link
Contributor Author

samccone commented May 31, 2017 •
edited
Loading

Added some simple tests @paulirish.

To test the destroy tmp, I am going to have to do some more heavy handed refactors, let me know what you think so far before I keep going.

@paulirish
Copy link
Member

We discussed offline:

  • Don't install the .chrome property on the launcher instance. if anything it should be in the return object of launch()
  • move if (this.opts.userDataDir === undefined) { to inside of destroyTmp so you can test that without faking a close event and a this.chrome interface.
  • verify tmpdir is deleted when it should be and not deleted when it shouldnt be

@samccone samccone force-pushed the allow-custom-profile branch 2 times, most recently from f65eb0b to ad404e0 Compare June 2, 2017 22:30
paulirish
paulirish reviewed Jun 2, 2017
View reviewed changes
chrome-launcher/package.json Outdated
@@ -5,16 +5,19 @@
"scripts": {
"build": "tsc",
"dev": "tsc -w",
"test": "mocha --reporter dot test/**/*-test.js",
"test": "mocha -r ts-node/register --reporter dot test/**/*-test.ts",
Copy link
Member

Choose a reason for hiding this comment

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

expand to --require

@samccone
Enable passing of a custom userDataDir to launcher
48124c3 
* rename TMP_DIR to a more accurate name. Since this is a private
internal value, this is non-breaking.

Fixes #2291
@samccone
Convert tests for launcher to typescript
32fcb44
@samccone
Implement tests for chrome launcher.
0fe2fa8 
* Refactor code bits to make testable.
* Add the ability for a user to pass in module mocks to enable testing.
paulirish
paulirish reviewed Jun 5, 2017
View reviewed changes
chrome-launcher/package.json Outdated
@@ -5,16 +5,19 @@
"scripts": {
"build": "tsc",
"dev": "tsc -w",
"test": "mocha --reporter dot test/**/*-test.js",
"tests": "mocha -r ts-node/register --reporter dot test/**/*-test.ts",
Copy link
Member

Choose a reason for hiding this comment

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

why remove yarn test ? Would prefer to continue the convention..

Copy link
Member

Choose a reason for hiding this comment

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

tests -> test. here and in run_mocha

chrome-launcher/package.json Outdated
@@ -5,16 +5,19 @@
"scripts": {
"build": "tsc",
"dev": "tsc -w",
"test": "mocha --reporter dot test/**/*-test.js",
"tests": "mocha -r ts-node/register --reporter dot test/**/*-test.ts",
Copy link
Member

Choose a reason for hiding this comment

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

can you use --require instead of -r ?

paulirish
paulirish approved these changes Jun 5, 2017
View reviewed changes
Copy link
Member

@paulirish paulirish left a comment

Choose a reason for hiding this comment

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

lgtm % last comments.

@samccone
Copy link
Contributor Author

samccone commented Jun 5, 2017

updated

@paulirish paulirish merged commit a9f5785 into master Jun 5, 2017
@paulirish paulirish deleted the allow-custom-profile branch June 5, 2017 22:24
@samccone
Copy link
Contributor Author

samccone commented Jun 6, 2017

Thanks everyone for the reviews

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@paulirish paulirish paulirish 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.
3 participants
@samccone @paulirish @wardpeet