Skip to content

Conversation

@kainino0x
Copy link
Collaborator

@kainino0x kainino0x commented Jul 19, 2025

3 separately reviewable commits

Probably needs more testing

Issue: #4424


Requirements for PR author:

  • All missing test coverage is tracked with "TODO" or .unimplemented().
  • New helpers are /** documented */ and new helper files are found in helper_index.txt.
  • Test behaves as expected in a WebGPU implementation. (If not passing, explain above.)
  • Test have be tested with compatibility mode validation enabled and behave as expected. (If not passing, explain above.)

Requirements for reviewer sign-off:

  • Tests are properly located in the test tree.
  • Test descriptions allow a reader to "read only the test plans and evaluate coverage completeness", and accurately reflect the test code.
  • Tests provide complete coverage (including validation control cases). Missing coverage MUST be covered by TODOs.
  • Helpers and types promote readability and maintainability.

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

@kainino0x
Copy link
Collaborator Author

@greggman just a draft but WDYT

@greggman
Copy link
Contributor

lgtm

are you planning to go through and refactor all the current tests doing late filtering?

I'm not sure about the skipIf_____ubound pattern. It works but seems convoluted? Like how about just

skipIfNotX(device: GPUDevice, params) { ... }

And then

class GPUTestBase {
   skipIfNotX(params) { skipIfNotX(this.device, params); }
}

Though I'm not against the ___ubound pattern. I just had to dig through the code to figure out why skipIf.. was being setup this way, and why the unusual naming pattern. It's not that different to just give it a normal name with normal parameters and call it.

@greggman
Copy link
Contributor

And also, do we want to discourage late filtering? Maybe we should require these to be called between init and postInit in AllFeaturesMaxLimitsGPUTest?

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