-
Notifications
You must be signed in to change notification settings - Fork 183
Add WebGL browser tests to vello_sparse_tests
#1020
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
Conversation
2871242 to
b785ab9
Compare
60ca4f9 to
8b4fe1c
Compare
40c53e5 to
336c606
Compare
vello_sparse_tests
vello_sparse_testsvello_sparse_tests
| wasm-bindgen = "0.2.100" | ||
|
|
||
| [features] | ||
| webgl = ["vello_hybrid/webgl"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This ensures that we're testing the vello_hybrid WebGL2 rendering backend by activating the webgl feature.
336c606 to
86e9573
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added the vello hybrid webgl tests and LFS. Without LFS the build system can't find any images to compile into the WASM test binary.
|
cc/ @LaurenzV (no action required from you), just wanted to let you know that your test framework was very readable and easy to extend to support browser tests. |
|
I'm not sure what changed, but when I run |
|
This is probably not for you to figure out, but at least on my system running the tests with Which surprises me, because we do have the |
I’ll definitely address that! Sorry and thank you for picking that up. It’s awkward that the easiest way to pass all the tests is to have zero tests :p Also that’s extremely strange that you are getting an error regarding wasm32. |
Fixed in 5009e2c |
5009e2c to
7c52996
Compare
|
Thanks @LaurenzV for the initial very great review.
|
Do other existing examples work? For example from within |
Yes, this test works fine for me. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems fine to me overall, but I'll leave the final review to someone more knowledgeable in WebGL. Perhaps @DJMcNab also wants to take a look at the CI side of things.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few threads of discussion:
- A potential way to avoid the build script (resolution of this is from my perspective blocking - but resolution could mean "we decide to make no change here)
- A tweak for how the CI job is named
- Thoughts about running even more tests on CI (CPU only tests should be trivial, whereas the "wgpu" hybrid tests depend on ChromeDriver having WebGPU support). This can be deferred.
Co-authored-by: Daniel McNab <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This pretty much works as I'd expect - leaving final review to others who have more expertise for the web platform.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This pretty much works as I'd expect - leaving final review to others who have more expertise for the web platform.
Web platform changes LGTM.
I left a couple small comments - none blocking. Perhaps check in with @grebmeg to check whether Alex has any blocking outstanding comments before merge 🙏 .
| } | ||
|
|
||
| // vello_hybrid WebGL renderer backend. | ||
| #[cfg(all(target_arch = "wasm32", feature = "webgl"))] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have we given much thought to the pattern we use for target specific implementations? We're currently using the below approach:
#[cfg(TARGET_1)]
pub fn check_ref(...) { ... }
#[cfg(TARGET_2)]
pub fn check_ref(...) { ... }I suspect that, for better organisation and discovering all target implementations, something like the below may be better when we have separate function implementations. It's more code, but I think it might ease the addition of later targets (e.g. cpu webgl) and improves discoverability.
pub(crate) fn check_ref(
...
) {
#[cfg(TARGET_1)]
check_ref_wgpu(...)
#[cfg(TARGET_2]
check_ref_webgl(...)
}
#[cfg(TARGET_1)]
fn check_ref_wgpu(...) { ... }
#[cfg(TARGET_2)]
fn check_ref_webgl(...) { ... }I think when we have sufficiently complex scenarios, it might be worth using a module splitting approach. But perhaps for small functions, the above is better?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tracking this great refactor as followup work that I'll include in the PR resolving this issue: #1025 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work, @AndrewJakubowicz! 🎉 I don’t have any blocking comments. The issue with headless mode in Chrome seems related to something in my local configuration, so I think we can address it later.
Note: This PR currently branches off #1021 as there is a lint error on
mainand I wanted to ensure CI fully passed.Tip
This is ready to review and passes all tests in CI.
Context
This follows #1011 and is the testing portion. In order to ensure renderer parity and correctness, I've forked the
vello_hybridtests to also include a_hybrid_webglvariant that use:#[wasm_bindgen_test::wasm_bindgen_test]and run via the browser.As a demo of this PR, I locally introduced a "fake error" that reflects the image. After running `wasm-pack test --chrome --features webgl` from the `vello_sparse_tests` directory, I got the following very long webpage. (unfurl details section by clicking this text, and scroll to bottom of test results)
Features
check_refsfunction._hybridtests are unchanged. I've added a_hybrid_webgltest.check vello_hybrid webgl backend passes vello_sparse_tests suiteCI step that uses headless browser.These tests could be fancier, but they do the job right now. Debugging a failure isn't easy, however the diff can be manually retrieved and compared by a human by spinning up a browser.
The goal of having webgl tests automatically match the vello_hybrid tests is to keep the
wgpuandwebglrenderer backends in sync.Test plan
This PR changes no business logic and is the test.
Note: Browser WebGL backend tests currently pass all of the same tests as the vello_hybrid wgpu renderer. Can be seen in CI here: https://github.com/linebender/vello/actions/runs/15211363959/job/42786078261?pr=1020
I tested the functionality of this change by introducing intentional breakages and ensuring that headless tests failed, and diff images could be found on the browser when run without headless.
Risks
I'm not sure of any risks as this isn't user facing.
Manually testing this change
Pull branch down locally. Then navigate the
vello_sparse_testsand run:wasm-pack test --chrome --features webgl. Navigate to the browser URL and see 65 tests succeed.Followup work
#1026
#1025