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

wgsl: Add validation test for min/max builtin functions #3497

Merged
merged 4 commits into from
Mar 15, 2024

Conversation

toji
Copy link
Member

@toji toji commented Mar 13, 2024

Add validation test for min/max builtin functions

Starting off with a simple set of test to make sure I'm doing it right. :) Biggest question for me is for the bad args tests, should we be testing things like "both args are the same type and both are bad?"

Also, these two files differ by 2 chars. Feels like that should be combined, but I'm not sure what sort of patterns we have in place for doing so, or if it's actually desirable.


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.

@toji toji requested a review from dneto0 March 13, 2024 22:07
@toji toji force-pushed the min-max-validation branch from 5b78b31 to 4cb51aa Compare March 13, 2024 22:11
@toji toji force-pushed the min-max-validation branch from 4cb51aa to 5a1465e Compare March 14, 2024 16:29
@toji toji requested a review from zoddicus March 14, 2024 21:04
Copy link
Contributor

@dneto0 dneto0 left a comment

Choose a reason for hiding this comment

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

If this were 3 days I ago I would have said "perfect!".

But @ben-clayton had feedback for me for how to improve this kind of test.
See updated #3496 for his feedback and how I adapted.

  1. combine kBadArgs and kGoodArgs into one dictionary kArgCases, where there is one good case named 'good'
  2. the 'bad_args' test becomes 'args'
  3. I also modified the must_use case to have both good and bad cases, parameterizing it on 'use' over false and true.

@dneto0
Copy link
Contributor

dneto0 commented Mar 14, 2024

Add validation test for min/max builtin functions

Starting off with a simple set of test to make sure I'm doing it right. :) Biggest question for me is for the bad args tests, should we be testing things like "both args are the same type and both are bad?"

I don't think that's needed. As long as there's at least one bad case for each argument position, that's ok. No way we want to do the entire power set.

Also, these two files differ by 2 chars. Feels like that should be combined, but I'm not sure what sort of patterns we have in place for doing so, or if it's actually desirable.

I like keeping the two separate. Makes it really easy to spot all (actually most) of the functionality for a given builtin.

E.g. just today @jrprice told me there was already centralized must-use testing for a bunch of builtins. But when I added the dp4a validation I didn't know about it. We decided to move all must-use testing for builtins into per-builtin tests, like we have here. #3502

@toji toji force-pushed the min-max-validation branch from 5a1465e to 7e9fdbb Compare March 14, 2024 22:24
@toji
Copy link
Member Author

toji commented Mar 14, 2024

Thanks for the feedback! Updated both tests to match the newer patterns you pointed out.

Copy link
Contributor

@dneto0 dneto0 left a comment

Choose a reason for hiding this comment

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

Thanks!

@dneto0 dneto0 force-pushed the min-max-validation branch from 7e9fdbb to c16b531 Compare March 15, 2024 17:07
@dneto0 dneto0 enabled auto-merge (squash) March 15, 2024 17:07
@toji toji force-pushed the min-max-validation branch 2 times, most recently from 0b2a499 to 84e584b Compare March 15, 2024 20:36
@toji toji force-pushed the min-max-validation branch from 52cadd8 to fee7357 Compare March 15, 2024 21:35
@dneto0 dneto0 merged commit 06cae14 into main Mar 15, 2024
1 check passed
@dneto0 dneto0 deleted the min-max-validation branch March 15, 2024 21:38
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