gpuweb / cts

WebGPU Conformance Test Suite
https://gpuweb.github.io/cts/
BSD 3-Clause "New" or "Revised" License
130 stars 82 forks source link

fixture.ts is checking for GPUPipelineError.stack to exist but it is not required on DOMException #3222

Open mwyrzykowski opened 11 months ago

mwyrzykowski commented 11 months ago

On this line: https://github.com/gpuweb/cts/blob/7a6ef7301b5d84f751b483f9d5466b3696749c26/src/common/framework/fixture.ts#L254C11-L254C11

we see a check for:

if (!(ex instanceof Error && typeof ex.stack === 'string')) {

however neither GPUPipelineError as defined in the specification nor DOMException are required to have such a field.

https://webidl.spec.whatwg.org/#es-DOMException-specialness says that:

if an implementation gives native Error objects special powers or nonstandard properties (such as a stack property), it should also expose those on DOMException objects.

Which uses the word 'should' and not 'shall' or 'must', meaning it is recommended but not required by the specification.

In any case, the WebGPU tests expecting a stack property on DOMException seems outside the scope of these tests.

Would it be acceptable to remove this check for ex.stack in the CTS? It does not seem to be doing anything useful.

mwyrzykowski commented 11 months ago

It looks like this was changed by https://github.com/gpuweb/cts/pull/3105 cc @kainino0x

I would propose the WebGPU CTS does not test this as it does not appear required by any standard.

kainino0x commented 10 months ago

Apologies for the delay on this, fell off my plate before the holidays.

I didn't dig into the specs because I didn't expect something this foundational to be unspecified behavior. I actually would strongly like to keep these checks, because I think it's a real source of inter-browser incompatibility if stack exists on some browsers and not others. IMO sometimes the tests can be aspirational and go beyond what the specs strictly require (though this philosophy is more applicable to things like the behavior of anisotropic filtering, than error stacks).

However if you think these should not be required then I can at least relegate the check to a single test, or a warning, or something, instead.

mwyrzykowski commented 10 months ago

No worries, it is not blocking as it is simple to work around. However to quote the readme of the CTS:

The contents of this test suite are considered normative; implementations must pass them to be WebGPU-conformant. Mismatches between the specification and tests are bugs.

unless the WebGPU specification specifically requires the stack property on GPUPipelineError, GPUInternalError, and GPUValidationError, which it currently does not, then testing for stack in the CTS seems the wrong place as the specification for DOMException does not require a stack property either. It seems contradictory to have aspirational tests in the CTS yet also state an implementation must pass the suite to be WebGPU-conformant.

A warning sounds great, but no rush as I just worked around it locally to ensure we didn't have other bugs preventing those suites from passing.