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

[RFC] Warden test framework #1589

Merged
merged 10 commits into from
Oct 19, 2017
Merged

[RFC] Warden test framework #1589

merged 10 commits into from
Oct 19, 2017

Conversation

kvark
Copy link
Member

@kvark kvark commented Oct 18, 2017

This is a pre-alpha of the new test infrastructure. It's based on RON-defined tests that are de-serialized and executed on each available native backend. See readme for devil in the details.

For Unix machines, all you need is make reftests.
For Windows (without WSL): (cd src/warden && cargo run --features vulkan,dx12,logger)
Output:

Warding Vulkan:
        AdapterInfo { name: "Radeon(TM) RX 460 Graphics ", vendor: 4098, device: 26607, software_rendering: false }
        Loading scene 'basic':
                Test 'render-pass-clear' ...    ran: PASS
Warding DX12:
        AdapterInfo { name: "Radeon(TM) RX 460 Graphics", vendor: 4098, device: 26607, software_rendering: false }
        Loading scene 'basic':
                Test 'render-pass-clear' ...    ran: PASS

Fixes #1410
Note: it will really shine when:

  1. we get GL support - Place of GL backend in the new low-level world #1539
  2. hook Warden up with headless glutin context - Testing infrastructure #1410
  3. run on Travis CI

HAL changes:

  • Instance specifies Backend as an associated type
  • create_renderpass -> create_render_pass for naming consistency

The PR has a lot of unfinished bits (e.g. PSO creation) but it's functional and technically ready for merging. I'm not 100% confident that being data-driven is a good thing here, hence RFC in the subject:

  • (+) RON is more expressive (maps, enum values, etc)
  • (+) somewhat consistent with Wrench
  • (-) RON is another format/language (note: can do any serde-supported format, really)
  • (~) run-time checked instead of compile-time. This is not really a great point since we are supposed to run those tests everywhere all the time, so we shouldn't end up in a situation where someone built them but didn't run.

@kvark kvark requested a review from msiglreith October 18, 2017 18:07
@torkleyy
Copy link
Contributor

This is awesome!

Copy link
Contributor

@msiglreith msiglreith left a comment

Choose a reason for hiding this comment

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

Awesome! 😍

download_type: hal::MemoryType,
}

fn align(x: usize, y: usize) -> usize {
Copy link
Contributor

Choose a reason for hiding this comment

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

What is this function doing?
edit: rather, how does this work?

let copy_submit = {
let mut cmd_buffer = command_pool.acquire_command_buffer();
let image_barrier = hal::memory::Barrier::Image {
states: image.stable_state .. (i::TRANSFER_READ, i::ImageLayout::TransferSrcOptimal),
Copy link
Contributor

Choose a reason for hiding this comment

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

Image needs to be created with TRANSFER_SRC usage.

@kvark
Copy link
Member Author

kvark commented Oct 19, 2017

fn align(x: usize, y: usize) -> usize {
    if x > 0 && y > 0 {
        ((x - 1) | (y - 1)) + 1
    } else {
        x
    }
}

It aligns address x to alignment y.
If x == 0 or y == 0 then we return x.
else if x % y == 0 then (x-1) | (y-1) would just be x-1 (assuming y is pow2), and then we add 1 which gives us x again. Otherwise (x-1) | (y-1) + 1 gives us the next number that is divisible by y.

It may be sub-optimal, or obscure, but it doesn't matter here as long as it's correct. Patches welcome ;)

Image needs to be created with TRANSFER_SRC usage.

We currently use the exact flags provided by the user. We should probably either add that usage if we know it's needed, or at least check for its presense, as a follow-up.

Also, since there seem to be an agreement over the fact that we need this (:tada:), I'm releasing bors :shipit:
bors r=msiglreith

@kvark
Copy link
Member Author

kvark commented Oct 19, 2017

Hmm, bors didn't get the hint. Let me try again.

bors r=msiglreith

@bors
Copy link
Contributor

bors bot commented Oct 19, 2017

Merge conflict

@kvark
Copy link
Member Author

kvark commented Oct 19, 2017

Rebased now

bors r=msiglreith

bors bot added a commit that referenced this pull request Oct 19, 2017
1589: [RFC] Warden test framework r=msiglreith a=kvark

This is a pre-alpha of the new test infrastructure. It's based on [RON](https://github.com/ron-rs/ron)-defined tests that are de-serialized and executed on each available native backend. See [readme](https://github.com/kvark/gfx/tree/warden/src/warden) for ~~devil in the~~ details.

For Unix machines, all you need is `make reftests`.
For Windows (without WSL): `(cd src/warden && cargo run --features vulkan,dx12,logger)`
Output:
```
Warding Vulkan:
        AdapterInfo { name: "Radeon(TM) RX 460 Graphics ", vendor: 4098, device: 26607, software_rendering: false }
        Loading scene 'basic':
                Test 'render-pass-clear' ...    ran: PASS
Warding DX12:
        AdapterInfo { name: "Radeon(TM) RX 460 Graphics", vendor: 4098, device: 26607, software_rendering: false }
        Loading scene 'basic':
                Test 'render-pass-clear' ...    ran: PASS
```

Fixes #1410
Note: it will really shine when:
  1. we get GL support - #1539 
  2. hook Warden up with headless glutin context - #1410 
  3. run on Travis CI

HAL changes:
  - `Instance` specifies `Backend` as an associated type
  - `create_renderpass` -> `create_render_pass` for naming consistency

The PR has a lot of unfinished bits (e.g. PSO creation) but it's functional and technically ready for merging. I'm not 100% confident that being data-driven is a good thing here, hence `RFC` in the subject:
-  (+) RON is more expressive (maps, enum values, etc)
-  (+) somewhat consistent with [Wrench](https://github.com/servo/webrender/tree/master/wrench/reftests)
-  (-) RON is another format/language (note: can do any `serde`-supported format, really)
-  (~) run-time checked instead of compile-time. This is not really a great point since we are supposed to run those tests everywhere all the time, so we shouldn't end up in a situation where someone built them but didn't run.
@bors
Copy link
Contributor

bors bot commented Oct 19, 2017

Build succeeded

@bors bors bot merged commit e10d1ef into gfx-rs:master Oct 19, 2017
@kvark kvark deleted the warden branch October 19, 2017 12:55
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.

3 participants