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

Skip test_multithreaded_compute on MoltenVK. #4096

Conversation

jimblandy
Copy link
Member

@jimblandy jimblandy commented Aug 29, 2023

Skip tests::test_multithreaded_compute in examples/hello-compute/src/tests.rs on MoltenVK for timeouts.

All commits should pass CI, but may be fine-grained enough that squashing makes sense. Up to you.

Various cleanups to expected failure code:

  • Give test image snapshots distinctive names.

    Include the adapter name and driver name in the filenames generated to store the -actual.png and -difference.png files holding screenshots of test program output.

    Previously, the filename only incorporated the backend, but since we test the Vulkan backend on both Mac and Windows, one was overwriting the other in the artifacts.

  • Upload GitHub workflow artifacts even when the test fails.

    When a test in .github/workflows/ci.yml's gpu-test fails, ensure that it uploads the artifacts anyway. These are screenshots and comparison images, so they're valuable in debugging the failure.

  • Allow expected test failures to match the driver name.

    Add a new field FailureCase::driver, which must match AdapterInfo::driver if given.

    Use this in TestParameters::molten_vk_failure.

  • Implement Default for FailureCase, and use it where appropriate.

    This makes it easier to add new fields to FailureCase in the future, without having to fix every use.

  • Make TestParameters::specific_failure take a FailureCase.

    Make TestParameters::specific_failure take a FailureCase struct as its sole argument, thus requiring callers to label each parameter with its meaning. This is clearer than using four positional parameters.

  • Introduce TestParameters::backend_adapter_failure.

    Introduce a new TestParameters builder method, backend_adapter_failure, for marking an expected failure on a specific adapter and backend. Use where appropriate.

  • Introduce TestParameters::adapter_failure_skip.

    Introduce a new TestParameters builder method for skipping a test on a specific adapter. Use where appropriate.

  • Update doc comments for some TestParameters methods.

    Replace references to specific_failures with outdated parameter lists with plain English explaining what each function actually does.

  • Use TestParameters::backend_failure method where appropriate.

Checklist

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

Connections
Link to the issues addressed by this PR, or dependent PRs in other repositories

Description
Describe what problem this is solving, and how it's solved.

Testing
Explain how this change is tested.

@jimblandy jimblandy force-pushed the test-multithreaded-compute-timeout-moltenvk branch from f115b76 to 849f333 Compare August 29, 2023 01:17
@jimblandy jimblandy requested a review from cwfitzgerald August 29, 2023 01:17
@jimblandy jimblandy force-pushed the test-multithreaded-compute-timeout-moltenvk branch from 849f333 to 255745e Compare August 29, 2023 19:53
Replace references to `specific_failures` with outdated parameter
lists with plain English explaining what each function actually does.
Introduce a new `TestParameters` builder method for skipping a test on
a specific adapter. Use where appropriate.
Introduce a new `TestParameters` builder method,
`backend_adapter_failure`, for marking an expected failure on a
specific adapter and backend. Use where appropriate.
@jimblandy jimblandy force-pushed the test-multithreaded-compute-timeout-moltenvk branch from 255745e to 9417623 Compare August 29, 2023 19:58
Make `TestParameters::specific_failure` take a `FailureCase` struct as
its sole argument, thus requiring callers to label each parameter with
its meaning. This is clearer than using four positional parameters.
This makes it easier to add new fields to `FailureCase` in the future,
without having to fix every use.
@jimblandy jimblandy force-pushed the test-multithreaded-compute-timeout-moltenvk branch 3 times, most recently from 4aa95c4 to 7f6683a Compare August 29, 2023 22:25
Add a new field `FailureCase::driver`, which must match
`AdapterInfo::driver` if given.

Use this in `TestParameters::molten_vk_failure`.
@jimblandy jimblandy force-pushed the test-multithreaded-compute-timeout-moltenvk branch from 7f6683a to afcfcd8 Compare August 29, 2023 22:37
Skip `tests::test_multithreaded_compute` in `examples/hello-compute/src/tests.rs` on MoltenVK for timeouts.
Include the adapter name and driver name in the filenames generated to
store the `-actual.png` and `-difference.png` files holding
screenshots of test program output.

Previously, the filename only incorporated the backend, but since we
test the Vulkan backend on both Mac and Windows, one was overwriting
the other in the artifacts.
When a test in `.github/workflows/ci.yml`'s `gpu-test` fails,
ensure that it uploads the artifacts anyway. These are screenshots and
comparison images, so they're valuable in debugging the failure.
@jimblandy jimblandy force-pushed the test-multithreaded-compute-timeout-moltenvk branch from afcfcd8 to f203995 Compare August 30, 2023 00:01
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 is great stuff, have some nits, but good after

tests/src/lib.rs Outdated Show resolved Hide resolved
tests/src/lib.rs Outdated Show resolved Hide resolved
Use `&'static str` to represent adapter and driver names in
`FailureCase`. This requires us to convert to lower case each time we
match them against the adapter's strings, which is a heap allocation,
but this is not hot code, so it's better to be simple.
Give `TestParameters` two methods: `skip` and `expect_fail`,
both of which take a `FailureCase` as their sole argument,
indicating when the test should be skipped / expected to fail.

This means that the spine of each `TestParameters` builder expression
uses the word `skip` or `expect_fail`, rather than having a random
`bool` argument to some other method determine the difference.

Add a new method `FailureCase::applies_to(info)`, which decides
whether a given `FailureCase` applies to a given adapter running the
test.

Simplify logic for categorizing passes and failures, expected and
otherwise.
@jimblandy
Copy link
Member Author

Requesting a new review, since some of the new commits are non-trivial. None of the older commits are affected.

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.

Very nice!

@cwfitzgerald cwfitzgerald enabled auto-merge (squash) September 4, 2023 03:39
@cwfitzgerald cwfitzgerald merged commit 54a7f0e into gfx-rs:trunk Sep 4, 2023
bradwerth pushed a commit to bradwerth/wgpu that referenced this pull request Sep 19, 2023
@jimblandy jimblandy deleted the test-multithreaded-compute-timeout-moltenvk branch December 13, 2023 16:36
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.

2 participants