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] Allow environment variables definition when launching chromium #912

Merged

Conversation

BenoitZugmeyer
Copy link
Contributor

I'd like to launch Chromium with a specific timezone for my tests. On Linux and macOS, I just need to define the TZ environment variable to the wanted timezone. So I added a new puppeteer.launch() option to allow defining the environment variables of the forked process.

I read the contributing rules, but I failed to write a good unit test for Windows, because Chromium seems to ignore the TZ environment variable on this platform. If anyone have an idea for a more cross-platform unit test, I'd be happy to amend my PR. For now, the test is simply disabled on Windows.

FWIW, a similar issue is open at the chrome-launcher project.

@googlebot
Copy link

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.


  • If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
  • If your company signed a CLA, they designated a Point of Contact who decides which employees are authorized to participate. You may need to contact the Point of Contact for your company and ask to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the project maintainer to go/cla#troubleshoot.
  • In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again.

@BenoitZugmeyer
Copy link
Contributor Author

I signed it!

@googlebot
Copy link

CLAs look good, thanks!

@BenoitZugmeyer BenoitZugmeyer changed the title [feat] Allow environment varibles definition when launching chromium [feat] Allow environment variables definition when launching chromium Sep 29, 2017
@aslushnikov
Copy link
Contributor

Hi @BenoitZugmeyer, thank you for the pull request.

The code looks legit. However, I wonder if setting up process.env prior to launching browser would work for your use case?

const savedEnv = Object.assign({}, process.env);

process.env.TZ = 'America/Noronha';
const browser = await puppeteer.launch();

Object.assign(process.env, savedEnv); // Restore env

@BenoitZugmeyer
Copy link
Contributor Author

It is indeed possible to mutate the process.env, but I find it inconvenient for one major reason: In your code sample, since you are restoring the environment after waiting for puppeteer.launch to finish, the env is altered for any concurrent task going on. A better (but more inconvenent) solution would be:

const savedEnv = Object.assign({}, process.env);

process.env.TZ = 'America/Noronha';
const browserPromise = puppeteer.launch();
Object.assign(process.env, savedEnv); // Restore env

const browser = await browserPromise;

This is possible because puppeteer.launch is calling child_process.spawn synchronously.

Also, in my opinion, documenting the possibility to define the chromium environment variables this way is clearer than letting the user guess how to do it by mutating process.env.

test/test.js Outdated
// Disabled on Windows until we find a good use case on this platform. The
// TZ environment variable is currently ignored, see
// https://bugs.chromium.org/p/chromium/issues/detail?id=125614
(process.platform === 'win32' ? xit : it)('env option', SX(async function() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Lets not add this test - it adds more debt.
The patch is small and mostly does plumbing, so we can tolerate landing this without a test.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Alright, test removed! (commit amended)

@aslushnikov aslushnikov merged commit c225b93 into puppeteer:master Oct 5, 2017
@chux0519
Copy link

Why setting the time zone to "Asia/Shanghai" does not work, and eventually I use 'utc-8' instead?

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

4 participants