Skip to content

Conversation

@ErichDonGubler
Copy link
Member

@ErichDonGubler ErichDonGubler commented Dec 27, 2025

Issue: #4538

See also #4539.


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.

@ErichDonGubler ErichDonGubler added bug Something isn't working update An existing test needs to be updated due to a spec change labels Dec 27, 2025
@kainino0x kainino0x self-requested a review January 3, 2026 01:11
kainino0x
kainino0x previously approved these changes Jan 3, 2026
Comment on lines 320 to 323
{ numVariablesDelta: 0, useExtraBuiltinInputs: true },
{ numVariablesDelta: -1, useExtraBuiltinInputs: true },
{ numVariablesDelta: -3, useExtraBuiltinInputs: true },
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: Test -2 and -3 because those are the cases on either side of the boundary. (Helps catch off-by-one errors or if an implementation tries to merge two of the variables but not the third.)

Suggested change
{ numVariablesDelta: 0, useExtraBuiltinInputs: true },
{ numVariablesDelta: -1, useExtraBuiltinInputs: true },
{ numVariablesDelta: -3, useExtraBuiltinInputs: true },
{ numVariablesDelta: -2, useExtraBuiltinInputs: true },
{ numVariablesDelta: -3, useExtraBuiltinInputs: true },

Copy link
Member Author

Choose a reason for hiding this comment

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

Added the -2 case with the latest force-push to 7c3fdf7. I want to clarify, though: your suggestion block would strip the -1 and 0 cases, and I'm guessing that's not intentional. Is that right?

Copy link
Collaborator

Choose a reason for hiding this comment

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

IIUC (please let me know if I'm misunderstanding the test):

It used to test -1 and 0 because those were on either side of the boundary (-1 would give space for the builtins, 0 would not). Now the boundary is between -3 (max valid) and -2 (min invalid). I think it should be pretty safe to assume that if the implementation gets -2 correct, then it will get -1 and 0 correct as well.

To be fair this is not clear because of the structure of the test since it computes success in the test body instead of passing it along with each case. It would probably be a good idea to do a little refactor here and make success a parameter (_success: true / _success: false) so we can actually tell that the test is testing the boundaries it intends to.

Copy link
Member Author

@ErichDonGubler ErichDonGubler Jan 9, 2026

Choose a reason for hiding this comment

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

Makes sense. Changed cases (but not the _success refactor) as suggested with latest force-push to f9fd2cc.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, but wait: If this is compat, then -1 is still an interesting boundary case to test, isn't it? It seems like that's still interesting to keep. That also means that we can't naively add _success to the case parameters, because whether it should succeed or not would actually depend on how many built-ins are being added, which is variable.

Copy link
Member Author

Choose a reason for hiding this comment

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

Force-pushed to e0157af to include -1.

Copy link
Collaborator

Choose a reason for hiding this comment

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

very good point... I should have actually looked at the code again before suggesting things.

I pulled it locally and experimented with another fix idea. Couldn't push to your branch so I made a new PR here that includes your commits and mine. PTAL: #4555

@ErichDonGubler ErichDonGubler force-pushed the fragment_input_deductions-in-max_variables_count branch from 98ec99c to 7c3fdf7 Compare January 6, 2026 00:02
@github-actions
Copy link

github-actions bot commented Jan 6, 2026

Results for build job (at e0157af):

-webgpu:api,validation,render_pipeline,inter_stage:max_variables_count,input:* - 8 cases, 8 subcases (~1/case)
+webgpu:api,validation,render_pipeline,inter_stage:max_variables_count,input:* - 10 cases, 10 subcases (~1/case)
-TOTAL: 284495 cases, 2341917 subcases
+TOTAL: 284497 cases, 2341919 subcases

@ErichDonGubler ErichDonGubler force-pushed the fragment_input_deductions-in-max_variables_count branch 2 times, most recently from faf6b48 to f9fd2cc Compare January 9, 2026 03:33
@ErichDonGubler ErichDonGubler force-pushed the fragment_input_deductions-in-max_variables_count branch from f9fd2cc to e0157af Compare January 9, 2026 03:47
@kainino0x kainino0x dismissed their stale review January 9, 2026 05:05

i'm editing the PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working update An existing test needs to be updated due to a spec change

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants