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

Puppeteer E2E test: Switch to Chromium beta channel #25504

Merged
merged 1 commit into from
Feb 14, 2023

Conversation

LeviPesin
Copy link
Contributor

Related issue: #25503 (comment)

Description

OmahaProxy started to return incorrect results for stable Chromium channel.

@mrdoob mrdoob added this to the r150 milestone Feb 14, 2023
@mrdoob mrdoob merged commit 1492601 into mrdoob:dev Feb 14, 2023
@LeviPesin
Copy link
Contributor Author

It also can be seen at https://omahaproxy.appspot.com/ -- take a look on the branch_base_position column.

@LeviPesin LeviPesin deleted the patch-4 branch February 14, 2023 09:21
@LeviPesin
Copy link
Contributor Author

Ideally we should backport this commit to other affected branches -- those ones that have stale runs. Or maybe wait and hope that OmahaProxy developers will quickly fix the problem...

@LeviPesin
Copy link
Contributor Author

And in any case we should, I think, cancel all current runs in progress. /ping @mrdoob

@LeviPesin
Copy link
Contributor Author

Or maybe wait and hope that OmahaProxy developers will quickly fix the problem...

Not sure how to report the problem to its developers...

@mrdoob
Copy link
Owner

mrdoob commented Feb 14, 2023

Do they recommend people to use that website for CI?

@LeviPesin
Copy link
Contributor Author

Not sure -- but using OmahaProxy for getting snapshots' numbers is recommended in many guides, including Puppeteer's ones. We could report the problem to Puppeteer developers.

@LeviPesin
Copy link
Contributor Author

LeviPesin commented Feb 14, 2023

@mrdoob Please cancel https://github.com/mrdoob/three.js/actions/runs/4171311708 and https://github.com/mrdoob/three.js/actions/runs/4171560439 runs. (All other current in-progress runs include 15-minute timeout so they aren't a problem.)

@mrdoob
Copy link
Owner

mrdoob commented Feb 14, 2023

Done!

@LeviPesin
Copy link
Contributor Author

Thanks! Hopefully it will start working now.

@LeviPesin
Copy link
Contributor Author

It seems it works!

@LeviPesin
Copy link
Contributor Author

LeviPesin commented Feb 14, 2023

@mrdoob I've filed an issue at the Chromium bug tracker: https://crbug.com/1416025

Also, I have found in an another OmahaProxy-related issue there that they plan to turn OmahaProxy off completely -- not sure what we can replace it with...

@mrdoob
Copy link
Owner

mrdoob commented Feb 14, 2023

Bad news...

I can't generate screenshots locally.

Screenshot 2023-02-14 at 19 04 48

I'm getting black screenshots 😔

Screenshot 2023-02-14 at 19 05 57

@LeviPesin
Copy link
Contributor Author

LeviPesin commented Feb 14, 2023

Maybe it's related to #25459? Does it work without useMacOSARMBinary: true?

@mrdoob
Copy link
Owner

mrdoob commented Feb 14, 2023

That does fix it! I'll remove that flag.

@LeviPesin
Copy link
Contributor Author

@epreston Seems like we can't use the ARM build yet.

@Mugen87
Copy link
Collaborator

Mugen87 commented Feb 14, 2023

I'm trying to make a screenshot and get this now:

Using Chromium 111.0.5563.19 (revision 1097561, https://storage.googleapis.com/chromium-browser-snapshots/Mac/1097561/chrome-mac.zip), beta channel on mac
node:internal/errors:491
    ErrorCaptureStackTrace(err);
    ^

Error: spawn Unknown system error -86
    at ChildProcess.spawn (node:internal/child_process:413:11)
    at Module.spawn (node:child_process:757:9)
    at BrowserRunner.start (file:///Users/michael/Documents/workspace/three.js/node_modules/puppeteer-core/lib/esm/puppeteer/node/BrowserRunner.js:91:34)
    at ChromeLauncher.launch (file:///Users/michael/Documents/workspace/three.js/node_modules/puppeteer-core/lib/esm/puppeteer/node/ChromeLauncher.js:68:16)
    at async Server.main (file:///Users/michael/Documents/workspace/three.js/test/e2e/puppeteer.js:203:12) {
  errno: -86,
  code: 'Unknown system error -86',
  syscall: 'spawn'
}

Node.js v18.13.0

@LeviPesin
Copy link
Contributor Author

Not sure what it is -- it's seems like some random Node error when trying to spawn a process.

@Mugen87
Copy link
Collaborator

Mugen87 commented Feb 14, 2023

I get the error consistently every time I try to make the screenshot.

@LeviPesin
Copy link
Contributor Author

LeviPesin commented Feb 14, 2023

Found this issue: pa11y/pa11y#619. It seems from it that you should install Rosetta 2 so that it will work (using useMacOSARMBinary: false, I think it should work out-of-the-box for useMacOSARMBinary: true).

@Mugen87
Copy link
Collaborator

Mugen87 commented Feb 14, 2023

With useMacOSARMBinary: false, I get the above runtime error.

With useMacOSARMBinary: true, I get this:

Error: webgl_loader_gltf_lights: THREE.WebGLRenderer: Error creating WebGL context., another attempt...
Screenshot generated for file webgl_loader_gltf_lights
1 screenshots succesfully generated.

But the screenshot is black. I'm using a mac Mini with M2 Pro.

@LeviPesin
Copy link
Contributor Author

@Mugen87 Does it work with useMacOSARMBinary: false and Rosetta 2 installed?

@Mugen87
Copy link
Collaborator

Mugen87 commented Feb 14, 2023

TBH, I don't want to install Rosetta 2 on this system. I would rather use a VM for making the screenshot.

@epreston
Copy link
Contributor

@LeviPesin we've known for a week that the same 5 tests fail on ARM. You said it is related to fonts. I'll finish the testing tonight and put the update in #25450 with the rest of the notes.

@LeviPesin
Copy link
Contributor Author

Different tests fail -- if I understand correctly, all tests fail on @mrdoob's and @Mugen87's devices when using useMacOSARMBinary: true (with "Unable to create WebGL context" error).

@epreston
Copy link
Contributor

If possible, can we move off of builds of chromium that are intended for chromium developers. Use the npm package ?

@LeviPesin
Copy link
Contributor Author

LeviPesin commented Feb 14, 2023

We used stable Chromium until today when OmahaProxy broke.

@epreston
Copy link
Contributor

We use stable developer chromium is my understanding.

@epreston
Copy link
Contributor

Notice how the doob and the mugen are on 111. That's not released to public.

@LeviPesin
Copy link
Contributor Author

LeviPesin commented Feb 14, 2023

We used stable Chromium until today (there is no such thing as "developer" Chromium -- Puppeteer downloads snapshots only from one source). Currently we are using beta Chromium because OmahaProxy broke after the release of stable Chromium 110.0.5481.97 (released on today's night).

@epreston
Copy link
Contributor

Chromium 111 has not been released. Look at their screenshots. They flipped stable on the dev builds to 111.

You are using a build intended for chrome plugin developers looking to evaluate future releases.

@LeviPesin
Copy link
Contributor Author

Yes, currently we are using beta channel. But until today we used stable channel.

@epreston
Copy link
Contributor

How is that possible if you were not using the chromium development setup ? But the instructions you have followed, the setup in place, is for people developing for chrome, not on it.

@epreston
Copy link
Contributor

Highly recommend switching to a NPM package that tracks stable releases. Then you can choose when and how chromium moves: not someone else's interpretation of stable. You also remove the "single point of success" when the proxy does not want to behave. NPM is fairly redundant.

@LeviPesin
Copy link
Contributor Author

LeviPesin commented Feb 14, 2023

How is that possible if you were not using the chromium development setup ?

  1. Puppeteer's BrowserFetcher class (in both puppeteer-core and puppeteer packages) downloads a Chromium snapshot from the Chromium snapshot storage based on revision number passed to it.
  2. puppeteer package (not puppeteer-core) comes with a single snapshot revision number that it autoinstalls.
  3. OmahaProxy provides revision number for latest Chromium on different channels.
  4. Our script in test/e2e/puppeteer.js uses this number to download Chromium -- it takes the number from the stable channel.
  5. Last night OmahaProxy broke and started to produce incorrect revision number for stable-channeled Chromium, other channels unaffected.
  6. We have changed the channel from stable to beta.

@epreston
Copy link
Contributor

epreston commented Feb 14, 2023

I don't want to derail your efforts.

I'll leave you to it. I don't know the history and I know I'm missing something. Maybe the project is sponsored to be an early adaptor.

From the outside, I have these questions. When you have time, explain it slowly to me.

Why dynamically query the latest version ? Why not choose a release and move the dependency when it makes sense. What is currently in place puts you in the position to explore every day one release issue.

@LeviPesin
Copy link
Contributor Author

Why dynamically query the latest version ?

Mainly because that using the puppeteer package requires installation of 200 MB Chromium even if you don't run E2E tests on your machine. Because of that package.json was splitted into package.json and test/package.json, which was not very easy to use. Using puppeteer-core and dynamic download allowed to revert this.

@epreston
Copy link
Contributor

epreston commented Feb 14, 2023

That does not address the question. I understand wanting to use the version were we tell it "if and when" to download the chromium browser.

"puppeter-core is for people who are managing browsers versions themselves." Let's manage it.
https://github.com/puppeteer/puppeteer/#puppeteer-core

Lock the version into something (constant or package var) we can pick up in the config script.

If you want to use browserfetcher, the API let's you specify the version:
https://pptr.dev/api/puppeteer.browserfetcher

@LeviPesin
Copy link
Contributor Author

Lock the version into something (constant or package var) we can pick up in the config script.

That's actually is a good idea. I will do that.

@epreston
Copy link
Contributor

Yeah man, the demo the current structure is based on is optimised for not having to update the demo every few months. It may not be what you want for production. I had another bit to add but I forgot it when I got distracted... back in 10 minutes.

@epreston
Copy link
Contributor

Check the screenshots from the other issue, my mac still says mac_arm-1070065. That's what I will complete testing on.

@epreston
Copy link
Contributor

epreston commented Feb 14, 2023

I remembered the other thing.

Set a minimum node version in the package.json so you are not chasing random test and build issues. You can make tests fail by being on old node.

  "engines": {
    "node": ">=16.19.0"
  },

Of course, double check the value with what's used in CI. They should match.

@epreston
Copy link
Contributor

Let's break it down.

You want to structure it so you pin to "released" versions. You can do this any way you like using the helper or installing a package.

Personally, I would use the value from the "full" puppeteer package. You get the benefit of delayed download / avoided download if not used, while someone else has done the work to ensure it's release quality. It also gives you a sanity check. You can swap to normal puppeteer if you think puppeteer-core is doing something odd. They should be producing the same result in this configuration.

People don't update their node distribution until they have to. That causes it's own issues and inconsistencies. It looks like the CI sets a version around 16. Require at least that for the project in the package.json to get a consistent baseline.

It might be best to force a browser language and locale (en-US) during testing. I haven't been able to reproduce it yet, but where font rendering is concerned I've noticed subtle differences when I switch between languages. There's notes against some of the development channels about the limits of translation: https://www.chromium.org/getting-involved/dev-channel/

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.

4 participants