Add "arguments" deserialization tests for "script.callFunction" #34830
Conversation
wpt-pr-bot
requested review from
AutomatedTester,
jgraham,
juliandescottes,
shs96c and
whimboo
|
Looks perfect to me! |
whimboo
changed the title
| ) | ||
| async def test_primitive_values(bidi_session, top_context, argument): | ||
| result = await bidi_session.script.call_function( | ||
| function_declaration=f"(arg) => arg", |
There was a problem hiding this comment.
So the problem here is that we actually do not compare the result of the deserialization but a complete round trip including deserialization and serialization. In case of the serialization also be broken we wouldn't detect a failure for the deserialization.
As such we would need an assertion for a type check as part of the function declaration. If that passes the value also needs to be checked. Then the test will pass if no exceptionDetails are part of the return value for the script.callFunction call.
The same actually also applies to the other test.
There was a problem hiding this comment.
@sadym-chromium fyi whimboo is off for a few weeks, but I agree with his feedback here. Ideally this should only assert deserialization. I received a ping for this review today but I didn't see a new patch. I'm assuming the ping was sent before seeing the comment above.
There was a problem hiding this comment.
Yeah, I pinged before I saw the comment.
Ideally, we want to test only deserialization, but comparing sets, arrays, maps and objects on the backend is quite messy. So having we tested serialization, I guess it's acceptable trade-off just to check the types of non-pimitives on the server side, and check the "deserialized-serialized" values on the client side. I implemented this approach in the current revision. WDYT?
There was a problem hiding this comment.
| target=ContextTarget(top_context["context"]), | ||
| ) | ||
|
|
||
| assert result == nan_remote_value |
There was a problem hiding this comment.
Can we use recursive compare here as well?
There was a problem hiding this comment.
makes sense. Done
sadym-chromium
dismissed
whimboo’s stale review
According to @juliandescottes, @whimboo is off for a few weeks
script.callFunction