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

Add browser.profile setting and fix Firefox addArguments bug #226

Merged
merged 4 commits into from
Sep 27, 2021
Merged

Conversation

aomarks
Copy link
Member

@aomarks aomarks commented Sep 26, 2021

This PR adds a new browser.profile JSON config setting which can be used like this:

{
  "benchmarks": [
    {
      "url": "mybench.html",
      "browser": {
        "name": "firefox",
        "profile": "/Users/<username>/Library/Application Support/Firefox/Profiles/<profile-name>"
      }
    }
  ]
}

It also fixes a bug where addArguments was not being applied to Firefox, even though it was documented that it was supported.

For Chrome, it was previously supported and documented to use "addArguments": ["user-data-dir=<path>"] to achieve this same effect, but I found that the equivalent for Firefox ("-profile=<path>") caused Selenium to timeout trying to connect to the process. I wasn't able to figure out exactly why this was happening, but I did notice a dedicated setProfile method just for Firefox, which does work. So by adding the browser.profile setting, we now have a way to call this special API, plus the user doesn't need to remember the user-data-dir flag in Chrome.

Fixes #222

cc @guybedford

Copy link
Collaborator

@AndrewJakubowicz AndrewJakubowicz left a comment

Choose a reason for hiding this comment

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

LGTM.

A couple non blocking questions:

  • How was this tested?
  • Does this need any additional test coverage?
  • Not totally clear what the parsed.profile change in the SafariConfig interface is for. Is it compatibility with the Chrome/Firefox interfaces?

Thank you!

README.md Outdated
look for "Profile Path".

If there is an existing Chrome process using this profile, you must first
terminate it. You also need to close all open tabs, or sable the "Continue where
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: sable?

Copy link
Member Author

Choose a reason for hiding this comment

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

Oops, should be "disable". Fixed, thanks!

README.md Outdated
#### Firefox

To find your current profile location in Firefox, visit `about:support` and look
for "Profile Directory".
Copy link
Collaborator

Choose a reason for hiding this comment

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

For me it was under Profile Folder. Maybe different versions of Firefox? I tested on Firefox 92.0

Copy link
Member Author

Choose a reason for hiding this comment

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

Huh, interesting. On Linux it's "Directory" and on macOS it's "Folder" for the same version. That seems like some pretty unnecessary localization! Added both.

@guybedford
Copy link

I just tested this out here and it works perfectly for me. Thanks so much for filling in the gaps on this workflow @aomarks it's a huge help!

@aomarks aomarks force-pushed the rename-horizons branch 2 times, most recently from cc3bb9f to 20a2b34 Compare September 27, 2021 21:13
Base automatically changed from rename-horizons to main September 27, 2021 21:16
Copy link
Member Author

@aomarks aomarks left a comment

Choose a reason for hiding this comment

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

  • How was this tested?
    I tested it manually in Firefox/Chrome on Linux and macOS.
  • Does this need any additional test coverage?

It would be nice to have an automated test here, but I think it's pretty tricky. I think we'd need to serialize a profile for each browser somehow, and then have a way to figure out from the results if we actually launched with that profile or not. Maybe the profiles would have an extension installed that inject a value or something? I think I must punt for now 😬

  • Not totally clear what the parsed.profile change in the SafariConfig interface is for. Is it compatibility with the Chrome/Firefox interfaces?

I don't think this change is actually affecting SafariConfig, it's just an illusion because of the way GitHub automatically folds part a file when showing a diff. It's the configfile.ts file, but the parseBrowserObject function.

  • Not totally clear what the parsed.profile change in the SafariConfig interface is for. Is it compatibility with the Chrome/Firefox interfaces?

README.md Outdated
look for "Profile Path".

If there is an existing Chrome process using this profile, you must first
terminate it. You also need to close all open tabs, or sable the "Continue where
Copy link
Member Author

Choose a reason for hiding this comment

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

Oops, should be "disable". Fixed, thanks!

README.md Outdated
#### Firefox

To find your current profile location in Firefox, visit `about:support` and look
for "Profile Directory".
Copy link
Member Author

Choose a reason for hiding this comment

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

Huh, interesting. On Linux it's "Directory" and on macOS it's "Folder" for the same version. That seems like some pretty unnecessary localization! Added both.

@AndrewJakubowicz
Copy link
Collaborator

Oops! I was completely fooled by that code folding. Thanks for answering my questions.

@aomarks
Copy link
Member Author

aomarks commented Sep 27, 2021

Also threw in a fix to tests relating to chromedriver having updated to Chrome 94 while GitHub Actions is still on Chrome 93.

@aomarks aomarks merged commit 724dbd5 into main Sep 27, 2021
@aomarks aomarks deleted the profiles branch September 27, 2021 21:41
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.

Firefox use default profile
3 participants