Fix race condition in shader validation tests

[diegodelatoba]

Changes:

• Modified both expectCompileResult implementations to use a single
  eventualAsyncExpectation that executes GPU validation and compilation
  checks sequentially
• Changed let shaderModule to const shaderModule for better code quality
• Removed unnecessary non-null assertion operators

Testing:

Verified in WebKit with 500 consecutive test runs with no failures (previously
failed around iteration 15-304).

Note: This is a test infrastructure bug fix, not related to a spec change.
No CTS project tracker issue is required.

---

Requirements for PR author:

• All missing test coverage is tracked with "TODO" or .unimplemented().
  • N/A - This is a test infrastructure fix, not adding new test coverage
• New helpers are /** documented */ and new helper files are found in helper_index.txt.
  • N/A - No new helpers added
• Test behaves as expected in a WebGPU implementation. (If not passing, explain above.)
  • Yes - Tested extensively in WebKit with 500+ consecutive successful runs
• Test have be tested with compatibility mode validation enabled and behave as expected. (If not passing, explain above.)
  • Yes - All affected tests pass in WebKit's test suite

Requirements for reviewer sign-off:

• Tests are properly located.
• Test descriptions are accurate and complete.
• Tests provide complete coverage (including validation control cases). Missing coverage MUST be covered by TODOs.
• Tests avoid over-parameterization (see case count report).

When landing this PR, be sure to make any necessary issue status updates.

[mwyrzykowski]

@greggman is there a race in the current logic? We see errors print in different orders based on promise resolution and the spec says:

https://www.w3.org/TR/webgpu/#dom-gpudevice-poperrorscope says:

There is no guarantee of the ordering of promise resolution.

or am I missing something and is the test currently guaranteed to be deterministic?

[greggman]

I'm not seeing the race but I'm terrible at seeing races 😅

The first check just expects that eventually the compilation validation succeeds or fails. Regardless, it will immediately and synchronously return a shader module.

The 2nd check is that you can call shaderModule.getComplationInfo and eventually get a result, and, if you expected an error at least one message is an error message.

I don't see a race about which order those are expected happen. The test seems like it allows them happening in any order

@diegodelatoba where do you see the race?

[diegodelatoba]

I'm not seeing the race but I'm terrible at seeing races 😅

The first check just expects that eventually the compilation validation succeeds or fails. Regardless, it will immediately and synchronously return a shader module.

The 2nd check is that you can call shaderModule.getComplationInfo and eventually get a result, and, if you expected an error at least one message is an error message.

I don't see a race about which order those are expected happen. The test seems like it allows them happening in any order

@diegodelatoba where do you see the race?

@greggman From my understanding the race isn't about which check happens first since those can happen in any order. Perhaps "race" was not the correct wording here but the issue here is that currently there is non-deterministic error message printing in the test output.

What I found is that expectGPUError and eventualAsyncExpectation both write to the test recorder (this.rec.debug() / this.rec.validationFailed()). When they run in parallel, their output messages can interleave non-deterministically. This causes issues in WebKit's test suite which relies on deterministic error logging.

[greggman]

I'm not seeing the race but I'm terrible at seeing races 😅
The first check just expects that eventually the compilation validation succeeds or fails. Regardless, it will immediately and synchronously return a shader module.
The 2nd check is that you can call shaderModule.getComplationInfo and eventually get a result, and, if you expected an error at least one message is an error message.
I don't see a race about which order those are expected happen. The test seems like it allows them happening in any order
@diegodelatoba where do you see the race?

@greggman From my understanding the race isn't about which check happens first since those can happen in any order. Perhaps "race" was not the correct wording here but the issue here is that currently there is non-deterministic error message printing in the test output.

What I found is that expectGPUError and eventualAsyncExpectation both write to the test recorder (this.rec.debug() / this.rec.validationFailed()). When they run in parallel, their output messages can interleave non-deterministically. This causes issues in WebKit's test suite which relies on deterministic error logging.

ok, that makes more sense to me. I wouldn't expect to see logs get interleaved, there's still only one JS thread. But, the order individual tests get logged might be different.

I wonder if it would be better to order the outputs at a lower level. As it is, it looks like the code calls this.rec.validationFailed and similar things and those can get called in any order. Maybe we can do something there to handle this in a generic way everywhere? I don't know how many other places this might come up.

throwing out ideas

1. remove this.rec and require calling t.getRec which you'd have to call outside a callback

   // OLD
   this.eventualAsyncExpectation(async () => {
     ...
     this.rec.validationFailed(error);
   );

   // new
   const rec = this.getRec();
   this.eventualAsyncExpectation(async () => {
     ...
     rec.validationFailed(error);
   );

   This would allow getRec to return things that output in order.

   Or along the same lines, make eventualAsyncExpectation pass in a rec

   // new
   this.eventualAsyncExpectation(rec, async () => {
     ...
     rec.validationFailed(error);
   );

2. change eventualAsyncExpectation so it just adds functions to a list of things to do later and then calls them in order and awaits each one. Currently it calls the function immediately and puts the promise in an array to wait on. But maybe it could be changed put the function on an array then call each one in order.

I'm leaning toward something more like 1. There aren't that many calls to this.rec so maybe it would be a relatively small change to add internal order ids.

Or maybe we can just make this change for this test. I'd be nice to get @kainino0x's feedback.

[kainino0x]

The ordering of test logs is definitely not meant to be deterministic, because it can always depend on things in the browser that have nondeterministic orders. We can certainly make some common code more deterministic, but it'll always be possible to write a test with nondeterministic logs.

Let me make sure I'm understanding the issue correctly.

• WebKit uses the WPT "runtime" for the CTS?
• The WPT runtime only prints logs on failing tests

In which case you could only be running into log ordering on failing tests. So is the issue that tests that are expected to fail in WebKit have nondeterministic error logs, so your test infra can't verify that they are failing in the expected way? (If my memory is still accurate about how WebKit's failure expectations work)

[mwyrzykowski]

• WebKit uses the WPT "runtime" for the CTS?

At least not for the WebGPU CTS or many other WebKit tests

• The WPT runtime only prints logs on failing tests

Both passing and failing tests print logs.

In which case you could only be running into log ordering on failing tests. So is the issue that tests that are expected to fail in WebKit have nondeterministic error logs, so your test infra can't verify that they are failing in the expected way? (If my memory is still accurate about how WebKit's failure expectations work)

Like for https://gpuweb.github.io/cts/standalone/?q=webgpu:shader,validation,expression,binary,add_sub_mul:* for instance, we pass all the tests, but sometimes it seems we complete test case N + K before test case N (where N, K > 0)

So the pass lines are out of order and the expected result is not necessarily equal to a prior run.

But if this is not guaranteed or expected in general from WebGPU's CTS then I think it would be on WebKit to change our test runner.

[kainino0x]

• WebKit uses the WPT "runtime" for the CTS?

At least not for the WebGPU CTS or many other WebKit tests

I didn't realize this, I thought you ran the WPT-compatible build using your WPT infra. Is the code open source where I could take a look to understand a bit better? It'll be useful for me in the future.

Like for https://gpuweb.github.io/cts/standalone/?q=webgpu:shader,validation,expression,binary,add_sub_mul:* for instance, we pass all the tests, but sometimes it seems we complete test case N + K before test case N (where N, K > 0)

So the pass lines are out of order and the expected result is not necessarily equal to a prior run.

Ah, if it's just about the ordering of the "subcase ran" messages (as those seem to be the only logs in this test) then I think we can fix that generally. (Depending on how you run tests, there may also be "case passed" logs - those should already be in order, but if not that would be fixable too.)