chore: add UX tests around @jsii/kernel serializer errors (#3632)

Bring attention to error messages as part of the contract of the
`@jsii/kernel` serialization component, as such messages are currently a
known pain point (not specific enough, confusing, overloaded, ...).

Introducing snapshot tests, and quick win improvements to the existing
error messages where possible.

Fixes #3636 

---

By submitting this pull request, I confirm that my contribution is made under the terms of the [Apache 2.0 license].

[Apache 2.0 license]: https://www.apache.org/licenses/LICENSE-2.0

Author: RomainMuller

packages/@jsii/kernel/src/kernel.test.ts

@@ -344,7 +344,7 @@ defineTest('fails for invalid enum member name', (sandbox) => {
         '$jsii.enum': '@scope/jsii-calc-lib.EnumFromScopedModule/ValueX',
       },
     });
-  }).toThrow(/No enum member named ValueX/);
+  }).toThrow(/No such enum member: 'ValueX'/);
 });
 
 defineTest('set for a non existing property', (sandbox) => {
@@ -1688,7 +1688,7 @@ defineTest(
         args: [123, { incomplete: true }],
       });
     }).toThrow(
-      /Missing required properties for jsii-calc.TopLevelStruct: required, secondLevel/,
+      /Missing required properties for jsii-calc\.TopLevelStruct: 'required', 'secondLevel'/,
     );
   },
 );
@@ -1709,7 +1709,7 @@ defineTest(
         ],
       });
     }).toThrow(
-      /Missing required properties for jsii-calc.SecondLevelStruct: deeperRequiredProp/,
+      /\[as jsii-calc.SecondLevelStruct\] Missing required properties for jsii-calc\.SecondLevelStruct: 'deeperRequiredProp'/,
     );
   },
 );
@@ -1721,7 +1721,9 @@ defineTest('notice when an array is passed instead of varargs', (sandbox) => {
       method: 'howManyVarArgsDidIPass',
       args: [123, [{ required: 'abc', secondLevel: 6 }]],
     });
-  }).toThrow(/Got an array where a jsii-calc.TopLevelStruct was expected/);
+  }).toThrow(
+    /Value is an array \(varargs may have been incorrectly supplied\)/,
+  );
 });
 
 defineTest(
@@ -2020,7 +2022,7 @@ defineTest('ANY serializer: null/undefined', (sandbox) => {
 
 defineTest('ANY serializer: function (fails)', (sandbox) => {
   expect(() => serializeAny(sandbox, 'anyFunction')).toThrow(
-    /JSII Kernel is unable to serialize `function`. An instance with methods might have been returned by an `any` method\?/,
+    /Functions cannot be passed across language boundaries/,
   );
 });
 

packages/@jsii/kernel/src/kernel.ts

@@ -210,7 +210,7 @@ export class Kernel {
     );
 
     this._debug('value:', value);
-    const ret = this._fromSandbox(value, ti);
+    const ret = this._fromSandbox(value, ti, `of static property ${symbol}`);
     this._debug('ret', ret);
     return { value: ret };
   }
@@ -233,7 +233,12 @@ export class Kernel {
 
     this._ensureSync(`property ${property}`, () =>
       this._wrapSandboxCode(
-        () => (prototype[property] = this._toSandbox(value, ti)),
+        () =>
+          (prototype[property] = this._toSandbox(
+            value,
+            ti,
+            `assigned to static property ${symbol}`,
+          )),
       ),
     );
 
@@ -260,7 +265,7 @@ export class Kernel {
       () => this._wrapSandboxCode(() => instance[propertyToGet]),
     );
     this._debug('value:', value);
-    const ret = this._fromSandbox(value, ti);
+    const ret = this._fromSandbox(value, ti, `of property ${fqn}.${property}`);
     this._debug('ret:', ret);
     return { value: ret };
   }
@@ -282,7 +287,12 @@ export class Kernel {
 
     this._ensureSync(`property '${objref[TOKEN_REF]}.${propertyToSet}'`, () =>
       this._wrapSandboxCode(
-        () => (instance[propertyToSet] = this._toSandbox(value, propInfo)),
+        () =>
+          (instance[propertyToSet] = this._toSandbox(
+            value,
+            propInfo,
+            `assigned to property ${fqn}.${property}`,
+          )),
       ),
     );
 
@@ -310,7 +320,11 @@ export class Kernel {
       },
     );
 
-    const result = this._fromSandbox(ret, ti.returns ?? 'void');
+    const result = this._fromSandbox(
+      ret,
+      ti.returns ?? 'void',
+      `returned by method ${method}`,
+    );
     this._debug('invoke result', result);
 
     return { result };
@@ -343,7 +357,13 @@ export class Kernel {
     });
 
     this._debug('method returned:', ret);
-    return { result: this._fromSandbox(ret, ti.returns ?? 'void') };
+    return {
+      result: this._fromSandbox(
+        ret,
+        ti.returns ?? 'void',
+        `returned by static method ${fqn}.${method}`,
+      ),
+    };
   }
 
   public begin(req: api.BeginRequest): api.BeginResponse {
@@ -402,7 +422,13 @@ export class Kernel {
       throw e;
     }
 
-    return { result: this._fromSandbox(result, method.returns ?? 'void') };
+    return {
+      result: this._fromSandbox(
+        result,
+        method.returns ?? 'void',
+        `returned by async method ${method.name}`,
+      ),
+    };
   }
 
   public callbacks(_req?: api.CallbacksRequest): api.CallbacksResponse {
@@ -444,6 +470,7 @@ export class Kernel {
       const sandoxResult = this._toSandbox(
         result,
         cb.expectedReturnType ?? 'void',
+        `returned by callback ${cbid}`,
       );
       this._debug('completed with result:', sandoxResult);
       cb.succeed(sandoxResult);
@@ -682,7 +709,11 @@ export class Kernel {
           get: { objref, property: propertyName },
         });
         this._debug('callback returned', result);
-        return this._toSandbox(result, propInfo);
+        return this._toSandbox(
+          result,
+          propInfo,
+          `returned by callback property ${propertyName}`,
+        );
       },
       set: (value: any) => {
         this._debug('virtual set', objref, propertyName, {
@@ -694,7 +725,11 @@ export class Kernel {
           set: {
             objref,
             property: propertyName,
-            value: this._fromSandbox(value, propInfo),
+            value: this._fromSandbox(
+              value,
+              propInfo,
+              `assigned to callback property ${propertyName}`,
+            ),
           },
         });
       },
@@ -822,7 +857,11 @@ export class Kernel {
             },
           });
           this._debug('Result', result);
-          return this._toSandbox(result, methodInfo.returns ?? 'void');
+          return this._toSandbox(
+            result,
+            methodInfo.returns ?? 'void',
+            `returned by callback method ${methodName}`,
+          );
         },
       });
     }
@@ -1063,73 +1102,41 @@ export class Kernel {
     return typeInfo;
   }
 
-  private _toSandbox(v: any, expectedType: wire.OptionalValueOrVoid): any {
-    const serTypes = wire.serializationType(
+  private _toSandbox(
+    v: any,
+    expectedType: wire.OptionalValueOrVoid,
+    context: string,
+  ): any {
+    return wire.process(
+      {
+        objects: this.objects,
+        debug: this._debug.bind(this),
+        findSymbol: this._findSymbol.bind(this),
+        lookupType: this._typeInfoForFqn.bind(this),
+      },
+      'deserialize',
+      v,
       expectedType,
-      this._typeInfoForFqn.bind(this),
-    );
-    this._debug('toSandbox', v, JSON.stringify(serTypes));
-
-    const host: wire.SerializerHost = {
-      objects: this.objects,
-      debug: this._debug.bind(this),
-      findSymbol: this._findSymbol.bind(this),
-      lookupType: this._typeInfoForFqn.bind(this),
-      recurse: this._toSandbox.bind(this),
-    };
-
-    const errors = new Array<string>();
-    for (const { serializationClass, typeRef } of serTypes) {
-      try {
-        return wire.SERIALIZERS[serializationClass].deserialize(
-          v,
-          typeRef,
-          host,
-        );
-      } catch (e: any) {
-        // If no union (99% case), rethrow immediately to preserve stack trace
-        if (serTypes.length === 1) {
-          throw e;
-        }
-        errors.push(e.message);
-      }
-    }
-
-    throw new Error(
-      `Value did not match any type in union: ${errors.join(', ')}`,
+      context,
     );
   }
 
-  private _fromSandbox(v: any, targetType: wire.OptionalValueOrVoid): any {
-    const serTypes = wire.serializationType(
+  private _fromSandbox(
+    v: any,
+    targetType: wire.OptionalValueOrVoid,
+    context: string,
+  ): any {
+    return wire.process(
+      {
+        objects: this.objects,
+        debug: this._debug.bind(this),
+        findSymbol: this._findSymbol.bind(this),
+        lookupType: this._typeInfoForFqn.bind(this),
+      },
+      'serialize',
+      v,
       targetType,
-      this._typeInfoForFqn.bind(this),
-    );
-    this._debug('fromSandbox', v, JSON.stringify(serTypes));
-
-    const host: wire.SerializerHost = {
-      objects: this.objects,
-      debug: this._debug.bind(this),
-      findSymbol: this._findSymbol.bind(this),
-      lookupType: this._typeInfoForFqn.bind(this),
-      recurse: this._fromSandbox.bind(this),
-    };
-
-    const errors = new Array<string>();
-    for (const { serializationClass, typeRef } of serTypes) {
-      try {
-        return wire.SERIALIZERS[serializationClass].serialize(v, typeRef, host);
-      } catch (e: any) {
-        // If no union (99% case), rethrow immediately to preserve stack trace
-        if (serTypes.length === 1) {
-          throw e;
-        }
-        errors.push(e.message);
-      }
-    }
-
-    throw new Error(
-      `Value did not match any type in union: ${errors.join(', ')}`,
+      context,
     );
   }
 
@@ -1148,7 +1155,7 @@ export class Kernel {
   private _boxUnboxParameters(
     xs: any[],
     parameters: spec.Parameter[] | undefined,
-    boxUnbox: (x: any, t: wire.OptionalValueOrVoid) => any,
+    boxUnbox: (x: any, t: wire.OptionalValueOrVoid, context: string) => any,
   ) {
     parameters = [...(parameters ?? [])];
     const variadic =
@@ -1166,7 +1173,9 @@ export class Kernel {
         })`,
       );
     }
-    return xs.map((x, i) => boxUnbox(x, parameters![i]));
+    return xs.map((x, i) =>
+      boxUnbox(x, parameters![i], `of parameter ${parameters![i].name}`),
+    );
   }
 
   private _debug(...args: any[]) {

packages/@jsii/kernel/src/serialization.test.ts

@@ -4,6 +4,7 @@ import { TOKEN_REF } from './api';
 import { ObjectTable } from './objects';
 import {
   SerializationClass,
+  SerializationError,
   SerializerHost,
   SERIALIZERS,
 } from './serialization';
@@ -28,7 +29,6 @@ const host: SerializerHost = {
   findSymbol: jest.fn().mockName('host.findSymbol'),
   lookupType,
   objects: new ObjectTable(lookupType),
-  recurse: jest.fn().mockName('host.recurse'),
 };
 
 describe(SerializationClass.Any, () => {
@@ -41,16 +41,6 @@ describe(SerializationClass.Any, () => {
     }
   }
 
-  beforeEach((done) => {
-    (host.recurse as jest.Mock<any, any>).mockImplementation(
-      (x: any, type: OptionalValue) => {
-        expect(type).toEqual(TYPE_ANY);
-        return anySerializer.serialize(x, type, host);
-      },
-    );
-    done();
-  });
-
   describe(anySerializer.serialize, () => {
     test('by-value object literal', () => {
       expect(
@@ -94,7 +84,7 @@ describe(SerializationClass.Scalar, () => {
       test('rejects any value', () =>
         expect(() =>
           scalarSerializer.deserialize('nothing!', TYPE_VOID, host),
-        ).toThrow(/Encountered unexpected `void` type/));
+        ).toThrow(/Encountered unexpected void type!/));
     });
 
     const scalarTypes = [
@@ -115,7 +105,7 @@ describe(SerializationClass.Scalar, () => {
               example.type,
               host,
             ),
-          ).toThrow(`Expected a ${example.name}, got {`);
+          ).toThrow(SerializationError);
         });
 
         for (const validValue of example.validValues) {
@@ -133,11 +123,7 @@ describe(SerializationClass.Scalar, () => {
           test(`rejects: ${invalidValue}`, () =>
             expect(() =>
               scalarSerializer.deserialize(invalidValue, example.type, host),
-            ).toThrow(
-              `Expected a ${example.name}, got ${JSON.stringify(
-                invalidValue,
-              )} (${typeof invalidValue})`,
-            ));
+            ).toThrow(SerializationError));
         }
       });
     }