Bidi: Pass evaluation objects tests #3081
Conversation
There was a problem hiding this comment.
Pull request overview
This PR implements BiDi evaluation object serialization and deserialization support to enable passing of evaluation objects tests. The changes add the ability to serialize plain C# objects (including anonymous types and POCOs) to BiDi's LocalValue format and properly deserialize BiDi responses.
Key changes:
- Added object serialization support for passing C# objects as arguments in BiDi evaluations
- Improved JsonElement deserialization to handle BiDi-specific types
- Added handling for non-serializable BiDi types (WindowProxyProperties) to return null
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| lib/PuppeteerSharp/Bidi/BidiRealm.cs | Implements object serialization methods (IsAnonymousType, ToCamelCase, SerializePlainObject, SerializeValue) and updates FormatArgumentAsync to handle plain objects; adds BiDi type filtering in deserialization |
| lib/PuppeteerSharp.Nunit/TestExpectations/TestExpectations.local.json | Removes test expectation entry for evaluation.spec objects tests, indicating these tests now pass |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| foreach (var prop in properties) | ||
| { | ||
| var value = prop.GetValue(arg); |
There was a problem hiding this comment.
The SerializePlainObject method doesn't handle potential exceptions from prop.GetValue(arg). If a property getter throws an exception (for example, if it has required dependencies that aren't initialized), this could cause the serialization to fail unexpectedly. Consider wrapping the property access in a try-catch block to handle such cases gracefully.
| var value = prop.GetValue(arg); | |
| object value; | |
| try | |
| { | |
| value = prop.GetValue(arg); | |
| } | |
| catch (Exception) | |
| { | |
| // Skip properties whose getters throw to avoid failing the entire serialization. | |
| continue; | |
| } |
| double d when double.IsNegativeInfinity(d) => LocalValue.NegativeInfinity, | ||
| double d when double.IsNaN(d) => LocalValue.NaN, |
There was a problem hiding this comment.
Similar to the issue in FormatArgumentAsync, the SerializeValue method at line 647 uses IsClass check which will match strings. Since strings are reference types and IsClass returns true for them, this could cause strings to be incorrectly processed as plain objects. However, the earlier pattern matches should catch strings first, so this may work due to pattern matching order. Still, for clarity and safety, it would be better to explicitly exclude string types in this condition.
| } | ||
|
|
||
| return LocalValue.Object(dict); | ||
| } | ||
|
|
||
| private LocalValue SerializeValue(object value) | ||
| { | ||
| return value switch | ||
| { | ||
| null => LocalValue.Null, | ||
| string s => LocalValue.String(s), | ||
| bool b => LocalValue.Boolean(b), | ||
| int i => LocalValue.Number(i), | ||
| long l => LocalValue.Number(l), | ||
| float f => LocalValue.Number(f), | ||
| double d when double.IsPositiveInfinity(d) => LocalValue.Infinity, | ||
| double d when double.IsNegativeInfinity(d) => LocalValue.NegativeInfinity, | ||
| double d when double.IsNaN(d) => LocalValue.NaN, | ||
| double d => LocalValue.Number(d), | ||
| decimal m => LocalValue.Number(m), |
There was a problem hiding this comment.
The SerializeValue method doesn't handle IEnumerable types (arrays, lists, etc.). When an object property contains a collection, it will fall through to the default case at line 647-648, which will attempt to serialize it as a plain object using reflection. This will likely fail or produce incorrect results. Collections should be explicitly handled by recursively serializing their elements into a LocalValue.Array.
| type.IsSealed && | ||
| nameContainsAnonymousType && | ||
| type.GetCustomAttributes(typeof(System.Runtime.CompilerServices.CompilerGeneratedAttribute), false).Length > 0; | ||
| } | ||
|
|
||
| private static string ToCamelCase(string name) | ||
| { | ||
| if (string.IsNullOrEmpty(name) || char.IsLower(name[0])) | ||
| { |
There was a problem hiding this comment.
The ToCamelCase method doesn't handle single-character names correctly. When the name has only one character, name[1..] will result in an empty string, which works but could be more explicit. Additionally, if the name has length exactly 1, it should just return the lowercased character.
| } | ||
|
|
||
| return LocalValue.Object(dict); | ||
| } | ||
|
|
||
| private LocalValue SerializeValue(object value) | ||
| { | ||
| return value switch | ||
| { | ||
| null => LocalValue.Null, | ||
| string s => LocalValue.String(s), | ||
| bool b => LocalValue.Boolean(b), | ||
| int i => LocalValue.Number(i), | ||
| long l => LocalValue.Number(l), | ||
| float f => LocalValue.Number(f), | ||
| double d when double.IsPositiveInfinity(d) => LocalValue.Infinity, | ||
| double d when double.IsNegativeInfinity(d) => LocalValue.NegativeInfinity, | ||
| double d when double.IsNaN(d) => LocalValue.NaN, | ||
| double d => LocalValue.Number(d), | ||
| decimal m => LocalValue.Number(m), |
There was a problem hiding this comment.
The SerializeValue method has potential for infinite recursion when dealing with circular references in objects. Line 647 recursively calls SerializePlainObject for any class type, which then calls SerializeValue for each property. If an object has a property that references itself or creates a circular dependency, this will cause a stack overflow. Consider adding circular reference detection similar to how DeserializeToObjectInternal handles it.
No description provided.