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: Fix text-rendering differences #25450

Draft
wants to merge 8 commits into
base: dev
Choose a base branch
from

Conversation

LeviPesin
Copy link
Contributor

@LeviPesin
Copy link
Contributor Author

It doesn't really seem this change works...

@epreston
Copy link
Contributor

epreston commented Feb 6, 2023

@LeviPesin gday, e2e was failing all tests on the mac after 3 mins so I took a peek. Code is pushing an --enable-features=Vulkan flag. That doesn't look right. What's it required for ?

Also added a --enable-logging=stderr --v=1 to the build flags to get more detailed console messages during runs so I can pass along detailed reasons for failures.

Where do we tune the memory allocated for the chromium browser session ?

@epreston
Copy link
Contributor

epreston commented Feb 6, 2023

Your constant for mac_arm , in PLATFORMS is missing an R in the value.

should be:

const PLATFORMS = {
	linux: 'linux',
	mac: 'mac',
	mac_arm: 'mac_arm64',
	win32: 'win',
	win64: 'win64'
};

See: #25395

@epreston
Copy link
Contributor

epreston commented Feb 6, 2023

To get the native mac browser and avoid slow rosetta emulation (buggy because its never tested on arm because they have arm builds) looks like you need to opt into it:

const browserFetcher = new BrowserFetcher( { path: 'test/e2e/chromium', useMacOSARMBinary: true } );

After fixing the typo in the PLATFORMS constant and opting into arm builds (old BrowserFetcher lib ?) :

console.log( browserFetcher.platform() );

This will return mac_arm instead of mac when it's on ARM. In turn, that downloaded the chromium for mac_arm and started running tests.

Screenshot 2023-02-07 at 10 08 08 am

@epreston
Copy link
Contributor

epreston commented Feb 7, 2023

It's finally able to complete a full run. The messages at the end might not be what you want to do.

Screenshot 2023-02-07 at 11 11 09 am

webgl_multiple_elements_text error was:

Screenshot 2023-02-07 at 11 13 13 am

webgl_postprocessing_procedural error was:

Screenshot 2023-02-07 at 11 12 53 am

I am doing another pull and running again. I see there's been some more updates.

@LeviPesin
Copy link
Contributor Author

Your constant for mac_arm , in PLATFORMS is missing an R in the value.

To get the native mac browser and avoid slow rosetta emulation (buggy because its never tested on arm because they have arm builds) looks like you need to opt into it:
const browserFetcher = new BrowserFetcher( { path: 'test/e2e/chromium', useMacOSARMBinary: true } );

Thank you! Will fix those errors.

The problem why some examples fail in this PR (which I tried to fix but to no avail) is text rendering -- I tried using some flags and/or setting a custom font so that it overwrites the default system font but that doesn't seem to work. I will investigate more.

@epreston
Copy link
Contributor

epreston commented Feb 7, 2023

Ran again with all the latest updates.

2 Mods applied, one ommitted.

  • PLATFORMS
  • useMacOSARMBinary: true
  • remove --enable-features=Vulkan

Next test is all 3. Vulkan won't have much supports on the Macs.

Updating this comment with the results. It's at least completing but now we have 5 errors instead of 2.

Results Summary:

Screenshot 2023-02-07 at 1 04 43 pm

Error Captures:

Screenshot 2023-02-07 at 1 11 43 pm

Screenshot 2023-02-07 at 1 11 58 pm

Screenshot 2023-02-07 at 1 12 15 pm

Screenshot 2023-02-07 at 1 12 25 pm

@epreston
Copy link
Contributor

epreston commented Feb 7, 2023

Update: Ignore this

Also a new issue popped up.

  • When it is retrying the test, it doesn't say if it ran the test again or its status

Screenshot 2023-02-07 at 1 16 18 pm

Update: end of queue puts it further down as you said, ignore this one.

@LeviPesin
Copy link
Contributor Author

When it is retrying the test, it doesn't say if it ran the test again or its status

This is strange, it should... It can be significantly below that line -- because when making a new attempt it's added to the end of the queue.

@epreston
Copy link
Contributor

epreston commented Feb 7, 2023

3 Mods applied:

  • PLATFORMS
  • useMacOSARMBinary: true
  • remove --enable-features=Vulkan

First Test Run

Screenshot 2023-
02-07 at 1 51 53 pm

Second Test Run

Screenshot 2023-02-07 at 2 11 12 pm

Third Test Run

Screenshot 2023-02-07 at 2 38 38 pm

@epreston
Copy link
Contributor

epreston commented Feb 7, 2023

Looks like the vulkan is harmless.

@epreston
Copy link
Contributor

epreston commented Feb 7, 2023

When it is retrying the test, it doesn't say if it ran the test again or its status

This is strange, it should... It can be significantly below that line -- because when making a new attempt it's added to the end of the queue.

Ok. Checking logs again to confirm. You're right... it was further down.

@LeviPesin
Copy link
Contributor Author

LeviPesin commented Feb 7, 2023

The Vulkan flag was added in #25304 by @mrdoob. I'm not really sure about what the flags from that PR do (I think swiftshader is enabled by default for headless Chromium) but at least they fix the "no adapters error" for WebGPU on Linux.

@epreston
Copy link
Contributor

epreston commented Feb 7, 2023

Forcing swiftshader is forcing software emulation. For automated tests running in the cloud with no GPU, might be a good thing. Slow for local testing but, for one frame thats a matter of the milliseconds.

I have the same tests failing all runs. Hows this match up with the image updates and tolerances ?

@epreston
Copy link
Contributor

epreston commented Feb 7, 2023

I'm going to run the suite full native, raw, no swiftshader and post the results below so you have that data point.

Update:

Same 5. New message on webgl_modifier_simplifier

Screenshot 2023-02-07 at 2 54 25 pm

@LeviPesin
Copy link
Contributor Author

I have the same tests failing all runs. Hows this match up with the image updates and tolerances ?

The examples that are failing are exactly the examples that use many text -- and the difference is exactly in text rendering. I will think more about how to solve this...

@epreston
Copy link
Contributor

epreston commented Feb 7, 2023

Updated the raw test data. Is it a font thing ? I remember needing to supply fonts in a few formats to get the same look across browsers.

@epreston
Copy link
Contributor

epreston commented Feb 7, 2023

The test rig is filtering web requests for the three library so it can return a version where random replaced with a fixed value to get consistent results.

These examples might import from other places. Is it possible some of these could need filtering now or in the future ?

	{
		"imports": {
			"three": "../build/three.module.js",
			"three/addons/": "./jsm/"
		}
	}

@epreston
Copy link
Contributor

epreston commented Feb 7, 2023

Another one:

Looks like the filtering is only doing http: requests. What if someone where to swap that to https in an example ? Might be required for certain types of code now or in the future.

@epreston
Copy link
Contributor

epreston commented Feb 7, 2023

The same examples fail each time. I'm going to run each example directly on the Mac and post the results.

Maybe there will be a font not loaded or not compatible, some sort of message.

@LeviPesin
Copy link
Contributor Author

There was a discussion about Math.random() filtering in #24109. I'm not very convinced it is needed (it was said there that otherwise many screenshots could break when another Math.random() is inserted before a one)...

By the way, we can theoretically just set the JS random value when starting the browser (there is such a flag) and then remove the filtering and Math._random() code from deterministic-injection.js.

@epreston
Copy link
Contributor

epreston commented Feb 9, 2023

Half way through the Mac tests. My back was hurting so had to break from using the laptop. I'll send through the infos shortly.

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

2 participants