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

Adds max_dynamic_buffers_per_pipeline tests #2595

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

lokokung
Copy link
Contributor

@lokokung lokokung commented May 4, 2023

Issue: #230


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.)

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.

@lokokung lokokung requested review from kainino0x and austinEng May 4, 2023 21:21
@github-actions
Copy link

github-actions bot commented May 4, 2023

Previews, as seen when this build job started (7015005):
Run tests | View tsdoc

// resources-of-type-per-stage is in pipeline layout creation.
// One bind group layout can have a maximum number of each type of binding *per stage* (which is
// different for each type). Test that the max works, then add one more entry of same-or-different
// type and same-or-different visibility.
Copy link
Collaborator

Choose a reason for hiding this comment

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

All test description text should be in the .desc(), let's clean this up and move any useful info into there.


// One pipeline layout can have a maximum number of each type of dynamic buffer per pipeline. Test
// that the max works, then add one more bind group layout with a buffer of the same-or-different
// type and same-or-different visibility.
Copy link
Collaborator

Choose a reason for hiding this comment

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

same


// One bind group layout can have a maximum number of each type of dynamic buffer per pipeline. Test
// that the max works, then add one more buffer of the same-or-different type and same-or-different
// visibility.
Copy link
Collaborator

Choose a reason for hiding this comment

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

same

g.test('max_resources_per_stage,in_bind_group_layout')
.desc(
`
Test that the maximum number of bindings of a given type per-stage cannot be exceeded in a
single bind group layout.
- Test each binding type.
- Test that creation of a bind group layout using the maximum number of bindings works.
- Test that creation of a bind group layout using the maximum number of bindings + 1 fails.
- TODO(#230): Update to enforce per-stage and per-pipeline-layout limits on BGLs as well.`
Copy link
Collaborator

Choose a reason for hiding this comment

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

This test hasn't actually changed. Am I reading right that the test was already up-to-date despite, just outdated comment and TODO?

// One pipeline layout can have a maximum number of each type of dynamic buffer per pipeline. Test
// that the max works, then add one more bind group layout with a buffer of the same-or-different
// type and same-or-different visibility.
g.test('max_dynamic_buffers_per_pipeline,in_pipeline_layout')
Copy link
Collaborator

Choose a reason for hiding this comment

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

This file is createBindGroupLayout so it's odd to have pipeline layout tests here (I know there are already some). I think we should move all of the limit tests that apply to both createBindGroupLayout and createPipelineLayout to a new file? (and leave behind notes in both createBindGroupLayout and createPipelineLayout's test files)
Can be a followup PR.

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