[p5.js 2.0 Bug Report]: Visual Tests Failed

[pnngocdoan]

Most appropriate sub-area of p5.js?

• Accessibility
• Color
• Core/Environment/Rendering
• Data
• DOM
• Events
• Image
• IO
• Math
• Typography
• Utilities
• WebGL
• Build process
• Unit testing
• Internationalization
• Friendly errors
• Other (specify if possible)

p5.js version

No response

Web browser and version

No response

Operating system

No response

Steps to reproduce this

Steps:

1. When running npx vitest run the textWeight visual test failed:

FAIL |unit|  test/unit/visual/cases/typography.js > Typography > textWeight > can control non-variable fonts > matches expected screenshots
Error: Screenshots do not match! 

2. However, when I ran that test alone npx vitest run -t 'textWeight', some time it passed some time not:

[welcome]

Welcome! 👋 Thanks for opening your first issue here! And to ensure the community is able to respond to your issue, please make sure to fill out the inputs in the issue forms. Thank you!

[ksen0]

Thanks for raising this @pnngocdoan ! Some of the typography tests have been a flaky test before, and this is a problem that is looking for a volunteer. Are you interested in investigating or fixing this?

(One previously discussed possible fix has been to look into how fonts are loaded and if there's issues with that - loading fonts from google urls, or some other issue. This only seems to occur with text, so that's why the investigation could start with text loading.)

[pnngocdoan]

Hi @ksen, so sorry for taking so long to reply. I was out of town for my graduation. I’m currently applying for the grant to help build a better visual test suite so I’ve been exploring the overall testing infrastructure since I feel like p5.js already has a pretty comprehensive setup. I’ve been focusing on the comparison algorithms because initially I thought they might be behind the flakiness (I happened to find another bug and I’ll be raising an issue on this soon!). But you’re right that the problem seems to only affect text. I haven’t had the chance to look specifically into font loading yet as I’ve been catching up from travel and wrapping up my application, but I’d love to dig into it more soon.

[ksen0]

Thanks for the update @pnngocdoan ! No worries, please feel free to message here if you're ready to work on this at some point in the future.

In the meantime, if someone else is keen to get started, they are also welcome to comment and volunteer!

[sophyphile]

Hi, thanks for raising this issue!

I've been looking into the repo too as I am applying for the same grant - I wish you the best of luck with your proposal!

I wasn't able to replicate this particular issue, though I have some suggested changes, which I have tested locally - my additions haven't broken anything at the least! 😆

My analysis is that this issue is likely caused by asynchronous Web Font loading. There is a possibility that while the loadFont() function has been successfully resolved, the font binary is still being downloaded or parsed by the browser. Thus, the call to textFont() may end up using a fallback font, followed by the screenshot() function capturing the fallback rendering, resulting in non-deterministic test output.

To mitigate this, we can try using document.fonts.ready to ensure all defined fonts are downloaded and ready for render. We can add a call to requestAnimationFrame() to ensure the fonts have been rendered on the canvas before taking a screenshot.

An alternative mitigation would be to self-host the font files as with the other local fonts, at least where the test does not explicitly require the use of a Google Font URL. This would eliminate the need for the above code.

@ksen0 I noticed a couple other visual tests in typography.js also use Web Fonts besides textWeight. Are you aware of any test flakiness on these tests? Perhaps the fix can aim to resolve flakiness across all tests using Web Fonts!

[davepagurek]

We currently call .load() on the FontFace object, but possibly after adding it to the document there is more loading that could be done? We could possibly try awaiting document.fonts.ready after that:

p5.js/src/type/p5.Font.js

Lines 947 to 951 in d51184b

	// load if we need to
	if (face.status !== 'loaded') await face.load();
	// add it to the document
	document.fonts.add(face);

[sophyphile]

My understanding is that the while face.load() will load the font, there is a chance that the font is still not fully integrated into the browser's rendering pipeline when screenshot() runs.

I had a spin at adding the following lines and ran 50+ iterations of tests with no failures. While I feel confident about the fix, I would equally treat it with some scepticism as I had not been able to replicate the issue in the first place.

// ensure the font is ready to be rendered
await document.fonts.ready;
await new Promise(r => requestAnimationFrame(r));

Full commit:
https://github.com/sophyphile/p5.js/blob/9eeffcf7c35f5f6fc6f710e7fcf15c989e46eff7/src/type/p5.Font.js#L953-L955

[davepagurek]

Since CI seems to be able to reproduce this more often, maybe you could make a PR and I can try manually rerunning tests for the PR a few times to see if it keeps passing?

I'd also ideally start with just the document.fonts.ready promise without the additional requestAnimationFrame to try to determine the minimal fix. So if we can't get the test to fail with just that on CI, then maybe that's all we need, and otherwise we can do the same process with requestAnimationFrame too.

[sophyphile]

Thanks Dave, I'm happy to do that, so feel free to assign it to me and I'll make a PR.

[davepagurek]

Thanks @sophyphile!