Add a new test for updateRenderState() #24415
Conversation
webxr/render_state_update.https.html
Outdated
| assert(tempSession); | ||
| assert_not_equals(session, tempSession); | ||
| session.updateRenderState({ baseLayer : new XRWebGLLayer(tempSession, sessionObjects.gl), }); | ||
| assert_unreached("updateRenderState should fail when baseLayer is created with a different session"); |
There was a problem hiding this comment.
can we just assert_throws this?
svillar
force-pushed
the
render-state-update
branch
from
17f1b77
to
120f4f6
Compare
|
@Manishearth mind taking another look? Thanks! |
webxr/render_state_update.https.html
Outdated
| resolve(session.end().then(() => { | ||
| try { | ||
| session.updateRenderState({}); | ||
| assert_unreached("updateRenderState should fail when created with an ended session"); |
webxr/render_state_update.https.html
Outdated
| return new Promise((resolve, reject) => { | ||
| try { | ||
| session.updateRenderState({ inlineVerticalFieldOfView : Math.PI, }); | ||
| assert_unreached("updateRenderState should fail when specifying an inlineVerticalFieldOfView with immersive sessions"); |
webxr/render_state_update.https.html
Outdated
|
|
||
| let testFieldOfView = function(session, fakeDeviceController, t) { | ||
| return new Promise((resolve, reject) => { | ||
| assert_throws_dom('InvalidStateError', () => session.updateRenderState({ inlineVerticalFieldOfView : Math.PI, })); |
There was a problem hiding this comment.
This test already exists via: webxr/render_state_vertical_fov_inline.https.html
There was a problem hiding this comment.
Ah, and also throwing isn't technically compliant: https://immersive-web.github.io/webxr/#apply-the-pending-render-state
There was a problem hiding this comment.
I think they're testing two different things. I'm trying to test the updateRenderState() API (which indeed throws) while the test you mention is actually testing the apply render state if I am not wrong
There was a problem hiding this comment.
I disagree, step 11 from that algorithm https://immersive-web.github.io/webxr/#dom-xrsession-updaterenderstate
Doesn't mention any validation to inlineVerticalFieldOfView except for throwing if it's set and being applied to an immersive session.
Applying the pending render state is where any validation actually happens, and that happens by clamping.
There was a problem hiding this comment.
Actually... I just looked closer, and this is setting to an immersive session (Not sure why I assumed it was setting on an inline session...).
That is also already tested via: webxr/render_state_vertical_fov_immersive.https.html
There was a problem hiding this comment.
I disagree, step 11 from that algorithm https://immersive-web.github.io/webxr/#dom-xrsession-updaterenderstate
Doesn't mention any validation to inlineVerticalFieldOfView except for throwing if it's set and being applied to an immersive session.
Applying the pending render state is where any validation actually happens, and that happens by clamping.
Then I might be missunderstanding the specs. What I am trying to test is step 4 of updateRenderState(), that's before the step 11 you mention.
There was a problem hiding this comment.
Just to be crystal clear, I don't want to test the validation at all (as you said there are 2 other tests for that), I just want to test the first 5 steps of the update algorithm.
There was a problem hiding this comment.
Yeah; sorry, my initial understanding was wrong and I thought it was the step 11; however step 4 is still tested (though it doesn't use assert_throws and ~implements that itself) in this test: https://github.com/web-platform-tests/wpt/blob/master/webxr/render_state_vertical_fov_immersive.https.html
(Similar name to the one I initially linked, but replicates this test).
There was a problem hiding this comment.
OK, I understand that you prefer to remove it then. I'll understand silence as a confirmation :)
webxr/render_state_update.https.html
Outdated
| assert_not_equals(session.renderState.baseLayer, layer); | ||
| }); | ||
| } catch (err) { | ||
| assert_unreached("updateRenderState should not fail (actually not do anything) with no params"); |
There was a problem hiding this comment.
NIT: Copy paste error, please update
There was a problem hiding this comment.
Ups, I'll fix that.
|
General comment, please wrap all asserts in t.step(() => { // foo }; That's probably something we should look into in webxr_util.js at a future date (I think it has to do with how we resolve the function, but it may be an unavoidable side-effect of how the tests assert within a promise/callback that isn't directly part of the chain...) |
Sure, I'll do that. Thanks for the suggestion. |
|
Thanks for the patience. I was on holidays so I couldn't update this before. PTAL. |
| let gl = sessionObjects.gl; | ||
| try { | ||
| gl.makeXRCompatible().then(() => { | ||
| t.step(() => { |
There was a problem hiding this comment.
NitPick: Only the asserts really need to be in the t.step, though I don't think this hurts anything.
There was a problem hiding this comment.
I just basically did it because I saw other tests with whole code blocks inside it.
|
TC reports that |
|
@Manishearth @alcooper91 do we really need @klausw's and @toji's approvals? Or are there any other additional actions expected from my side? |
|
Alex's review should be fine |
Any idea why some checks are not apparently completed? |
|
Alex might not have merge access to this repo? But I do, and even with my review it's not working |
|
Retriggered the build |
Seems like it didn't work either |
|
Cc @stephenmcgruer ? |
|
Huh. This looks like it could be a Taskcluster bug - the actual task succeeded (https://community-tc.services.mozilla.com/tasks/groups/MKIoi4r8QxyD-pea0sz9kw are all green) but Taskcluster isn't reporting the status to GitHub... thanks for the tag - I'll admin merge and keep an eye out for more reports of this nature. |
Thanks @stephenmcgruer ! |
This PR adds a new test for the updateRenderState() algorithm described https://immersive-web.github.io/webxr/#dom-xrsession-updaterenderstate.