fix: improve guessing of chrome executable path on windows

[audiBookning]

Reupload: Corrected conventions on naming conventions

A PR for the case where the user windows OS is in a different language and the 'Typical Chrome Executable Path' is on a 'non-English-standard' ProgramFiles folder.
In windows, it can be gotten by the environment variable ProgramFiles(x86).

[B4nan]

Thanks for the suggestion, but let's first start with the problem you are trying to solve. Can you describe it more in detail? How can we test it ourselves?

The proposed change itself looks a bit funky, given the string you are using regexes on is static, I can't see any reason to actually do this via regexp.

You should be able to adjust the chrome path via APIFY_CHROME_EXECUTABLE_PATH env var, if it's something non-standard.

[audiBookning]

Can you describe it more in detail?

I was experimenting with this package as a prospect for a future project. I tried to add some non-existent feature to it but was blocked right away when the tests failed (ex: puppeteer_launcher.test - supports useChrome option ).

I can't see any reason to actually do this via regexp

Yes you are right. At the time i used this more general code to deal with any 'environment variable' on other places and didn't clean it latter. I can change it to something simpler like for example.

return `${process.env['ProgramFiles(x86)']}\\Google\\Chrome\\Application\\chrome.exe`

How can we test it ourselves?

There seem to be already some CI test with Windows OS in this repo. That may be the best path.

I see that they are failing. Some of the errors looks similar to a situation i had.
I am no sure how i managed to pass the tests. I was doing several things at the same time and windows doesn't help much...
But i think that installing Puppeteer (and/or Playwright?) globally did the trick.

Reason for my PR:
My PR would be helpful for people who might want to contribute to the package.

thanks for taking the time to give a quick feedback even on the week end 🙂
I will wait for your response before doing anything more.

[mnmkng]

AFAIK the path is dependent on the Chrome version. Maybe we should support a fallback to the old one.

[B4nan]

There seem to be already some CI test with Windows OS in this repo. That may be the best path.

Tests in the CI are passing on windows too in master, they are just too flaky, so failing rather randomly because of some timeouts, definitely not because of this. I believe they also work locally on windows, right @vladfrangu?

Seeing the tests in your branch, they actually fail because chrome is not installed on that path you changed. In other words, you fixed your local environment, but in CI it is installed elsewhere and it breaks. We should check if there is something on the path and fallback to the currently defined one. I would also like to see a fallback in case the env var is not set.

Did some googling and I would actually say that the problem is not about languages, you just installed 32 bit version of chrome, while normally it is 64 bit that lives in the path we have hardcoded.

https://stackoverflow.com/a/9608782/3665878

[audiBookning]

Yes the ProgramFiles(x86) is not the correct variable to be used here.
So

return `${process.env['ProgramFiles']}\\Google\\Chrome\\Application\\chrome.exe`

would be the correct one?

Note that Chrome installing on path C:\Program Files (x86) even on 64bit windows machines, is an old issue that was only solved by mid 2020, i think. This path is still normal for people that have installed their chrome browser before that date (like me).

[B4nan]

It helps with the CI tests, so as long as it works for you, I guess it's the right one :]

I would still like to see a fallback in case the env var is not present, so we preserve the old behaviour, other than that I am happy to merge.

Would appreciate if you could check this locally too @vladfrangu

[audiBookning]

I just saw my last commit and realized that it couldn't be correct. It was already very late, when i made it and a little tired.
Worse i ended up not running the tests :(
I changed the code with different languages in mind, as well as the Chrome issue and with the original default fallback.
The tests in my machine passed correctly this time.

[B4nan]

Thanks!