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

Test setup for WebAssembly+WebGL #3238

Merged
merged 34 commits into from
Dec 9, 2022

Conversation

haraldreingruber
Copy link
Contributor

@haraldreingruber haraldreingruber commented Nov 26, 2022

Checklist

  • Run cargo clippy.
  • Run RUSTFLAGS=--cfg=web_sys_unstable_apis cargo clippy --target wasm32-unknown-unknown if applicable.
  • Add change to CHANGELOG.md. See simple instructions inside file.

Connections
#3162

Description
This PR runs wasm-bindgen-tests in a headless-chrome (WebGL backend only).

@haraldreingruber haraldreingruber marked this pull request as draft November 26, 2022 22:05
@codecov-commenter
Copy link

codecov-commenter commented Nov 26, 2022

Codecov Report

Merging #3238 (3b34bcd) into master (88acaf7) will increase coverage by 0.03%.
The diff coverage is n/a.

❗ Current head 3b34bcd differs from pull request most recent head d79b16d. Consider uploading reports for the commit d79b16d to get more accurate results

@@            Coverage Diff             @@
##           master    #3238      +/-   ##
==========================================
+ Coverage   65.60%   65.63%   +0.03%     
==========================================
  Files          82       82              
  Lines       39468    39474       +6     
==========================================
+ Hits        25894    25910      +16     
+ Misses      13574    13564      -10     
Impacted Files Coverage Δ
wgpu-hal/src/lib.rs 26.21% <0.00%> (ø)
wgpu-core/src/lib.rs 96.55% <0.00%> (ø)
wgpu-hal/src/gles/mod.rs 61.53% <0.00%> (ø)
wgpu/src/lib.rs 51.50% <0.00%> (+0.07%) ⬆️
wgpu-core/src/instance.rs 66.01% <0.00%> (+0.09%) ⬆️
wgpu-core/src/validation.rs 59.01% <0.00%> (+0.13%) ⬆️
wgpu-core/src/hub.rs 60.82% <0.00%> (+0.15%) ⬆️
wgpu-hal/src/dx12/adapter.rs 81.71% <0.00%> (+0.22%) ⬆️
wgpu-hal/src/gles/adapter.rs 83.89% <0.00%> (+0.30%) ⬆️
wgpu-hal/src/gles/egl.rs 38.49% <0.00%> (+0.49%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

Cargo.toml Outdated Show resolved Hide resolved
wgpu/tests/wasm.rs Outdated Show resolved Hide resolved
wgpu/tests/wasm.rs Outdated Show resolved Hide resolved
Copy link
Member

@cwfitzgerald cwfitzgerald left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(I know you're probably not done yet, but some small comments)

Looking great!

wgpu/tests/clear_texture.rs Outdated Show resolved Hide resolved
wgpu/tests/common/mod.rs Outdated Show resolved Hide resolved
Copy link
Member

@cwfitzgerald cwfitzgerald left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Whoops wrong button

@codeart1st
Copy link

wasm-bindgen-test with headless chrome under linux is broken for now.

https://github.com/codeart1st/wgpu-layers/actions/runs/3558060356/jobs/5976466514
https://bugs.chromium.org/p/chromium/issues/detail?id=1346838

I only get it working with a real physical GPU, but software vulkan rendering is broken in chrome with llvmpipe or swiftshader.

Maybe you have more luck to get it working.

@cwfitzgerald
Copy link
Member

This PR will only be testing webgl - so we should be okay. With how unstable webgpu is anyway, I wouldn't want to commit to testing it yet.

@haraldreingruber
Copy link
Contributor Author

One thing I was not able yet to wrap my head around is, why clippy fails for the PR in the official repo, but it works (for the same commit) on my fork: https://github.com/haraldreingruber/wgpu/actions/runs/3604776801/jobs/6074498868

Here is the same job in upstream: https://github.com/gfx-rs/wgpu/actions/runs/3604776894/jobs/6074499138

Any ideas? Maybe a different cache content?

@cwfitzgerald
Copy link
Member

This is because your fork is running your code, and the PR is running against the result of merging your code into master. There are changes on master that cause the code to break. Merging in master to this branch will let you fix the problems.

Copy link
Member

@cwfitzgerald cwfitzgerald left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nits

.github/workflows/ci.yml Outdated Show resolved Hide resolved
.github/workflows/ci.yml Outdated Show resolved Hide resolved
by @cwfitzgerald

Co-authored-by: Connor Fitzgerald <connorwadefitzgerald@gmail.com>
@haraldreingruber
Copy link
Contributor Author

haraldreingruber commented Dec 5, 2022

Those look like tests that all rely on panics being catchable to work (they aren't on wasm).

Yes, that's correct. At least it is the case for buffer_copy.rs and texture_bounds.rs.

example_wgsl.rs is searching for *.wgsl files in the examples source folders, which would need to be done completely differently for wasm. Not sure if this test is actually interesting to test in the browser... Is it mainly testing the WGSL validator? Maybe it is safe to assume that the code should work equally for wasm if the test works for non-wasm?

clear_texture.rs tests raise texSubImage2D: invalid format warnings in the Chrome dev-console. Maybe we need to improve the checks (and/or validations) to catch these errors?

If we replace the panics with wrapping the panicing code with fail, it should prevent the validation errors from turning into panics.

I am not sure how to do that. I guess we only want to modify test code and don't want to change wgpu itself?
If you could point me to some starting position I would be happy to give it a try.
But maybe we should consider to finish up this PR and improve the wasm testing in a separate one, unless it could be a simple change?

@cwfitzgerald
Copy link
Member

I think the most reasonable is to fix the simple to fix problems and leave the more fundamental problems for later.

For the ones that panic for control flow:

For clear_texture.rs let's just leave it, and file an issue.

For example_wgsl.rs yeah it only makes sense on native.

Copy link
Member

@cwfitzgerald cwfitzgerald left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This, plus the minor failure stuff, then we're good to go! Thanks for bringing this to completion!

wgpu/tests/common/mod.rs Outdated Show resolved Hide resolved
wgpu/tests/common/mod.rs Outdated Show resolved Hide resolved
wgpu/tests/common/mod.rs Outdated Show resolved Hide resolved
@cwfitzgerald
Copy link
Member

cwfitzgerald commented Dec 7, 2022

Curious why headless firefox didn't work - it works locally?

@haraldreingruber
Copy link
Contributor Author

Curious why headless firefox didn't work - it works locally?

Yes, it works on my Windows machine.
Will try again in the CI script, just in case we fixed it with the other improvements...

@haraldreingruber
Copy link
Contributor Author

Will try again in the CI script, just in case we fixed it with the other improvements...

All tests fail with panicked at 'could not create surface from canvas: CreateSurfaceError', wgpu/tests/common/mod.rs:331:14

One guess would be that it's related to FF not working with software rendering in GH CI? On the other hand, I did also try running it with the "gfx.prefer-mesa-llvmpipe" config flag.
If this is not the issue, I could try to create another job with a windows runner?

@cwfitzgerald
Copy link
Member

Interesting, let's leave FF to a followup, it's not terribly important anyway.

@haraldreingruber
Copy link
Contributor Author

Wrapping https://github.com/gfx-rs/wgpu/blob/master/wgpu/tests/buffer_copy.rs#L18 in a call to fail_if(should_panic, || ...) and removing the if statement before should fix the problem.

Thanks! It worked for texture_bounds.rs.
For buffer_copy.rs it still fails with panicked at 'Error in Queue::write_buffer: copy size 17 does not respect COPY_BUFFER_ALIGNMENT', wgpu\src\backend\direct.rs:311:9

Any idea why this workaround doesn't work in this case but does for the other?
Could it have to do with how the panics and error_scopes are implemented in the production code?

@cwfitzgerald
Copy link
Member

Yeah that means it's not getting redirected to the error scope correctly - lets just skip that one and follow up later.

Copy link
Member

@cwfitzgerald cwfitzgerald left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Amazing work! I'll add a changelog entry, then I'll merge it!

@cwfitzgerald cwfitzgerald enabled auto-merge (squash) December 9, 2022 00:55
@cwfitzgerald cwfitzgerald merged commit f0f700c into gfx-rs:master Dec 9, 2022
@haraldreingruber
Copy link
Contributor Author

Thanks for all the support!
I've created #3282 for the remaining tests. Please let me know if anything is missing in the description.

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