wpt icon indicating copy to clipboard operation
wpt copied to clipboard

Published 20 hours ago • verified publisher icon web-platform-tests

Update script result_ownership tests to handle id is only on root

Open juliandescottes opened this issue 3 years ago • 3 comments

Per serialization spec, child ownership is always set to "none" before serializing values. This means that only the root can ever have a handle id set.

Our current tests for resultOwnership="root" assert that a handle is set on the root remote value, but not that nested remote values do not have a handle id.

juliandescottes avatar Sep 20 '22 13:09 juliandescottes

The question is do we want to restrict the protocol extensions. And having the nested handlers can be considered as such an extension.

sadym-chromium avatar Sep 20 '22 13:09 sadym-chromium

CC'ing @jgraham as well.

whimboo avatar Sep 20 '22 19:09 whimboo

The question is do we want to restrict the protocol extensions. And having the nested handlers can be considered as such an extension.

I imagine this a question about the spec rather than about the test modification? Should we file another issue for that? Otherwise I'm not sure when we need to consider extensions when writing tests?

juliandescottes avatar Sep 21 '22 06:09 juliandescottes

The question is do we want to restrict the protocol extensions. And having the nested handlers can be considered as such an extension.

@sadym-chromium I will file a separate issue for this topic, could we land this change?

juliandescottes avatar Feb 27 '23 14:02 juliandescottes

Note: I just rebased for now, will apply comments afterwards.

juliandescottes avatar Mar 13 '23 12:03 juliandescottes

@whimboo I applied the comments, can you take a look?

juliandescottes avatar Mar 13 '23 14:03 juliandescottes

Thanks for the review! I can see wpt-chrome-dev-stability is failing, if I understand correctly because of slow tests detected. None of the tests mentioned use the helper which was modified here, so it sounds unlikely that this is related to my patch.

I asked for the task to be re executed.

juliandescottes avatar Mar 13 '23 17:03 juliandescottes

Thanks for the review! I can see wpt-chrome-dev-stability is failing, if I understand correctly because of slow tests detected. None of the tests mentioned use the helper which was modified here, so it sounds unlikely that this is related to my patch.

This task is always failing those days and maybe it needs to be removed if no-one from Google could get it fixed. @sadym-chromium, @foolip any idea what's wrong with it?

whimboo avatar Mar 14 '23 07:03 whimboo

@jgraham could you please merge this PR? Thanks.

whimboo avatar Mar 14 '23 16:03 whimboo