Troubleshoot and fix issues in grpc-web JS conformance client

Makefile

@@ -110,11 +110,7 @@ runclienttests: $(BIN)/connectconformance $(BIN)/referenceclient $(BIN)/grpcclie
 	$(BIN)/connectconformance -v --conf ./testing/grpc-impls-config.yaml --mode client --trace \
 		--known-failing @./testing/grpcclient-known-failing.txt \
 		-- $(BIN)/grpcclient
-	@# Note that use of --skip is discouraged, but if we don't skip them the client crashes.
-	@# TODO: troubleshoot the skipped test cases and figure out why they crash.
 	$(BIN)/connectconformance -v --conf ./testing/grpc-web-client-impl-config.yaml --mode client --trace \
-		--skip "**/trailers-only/missing-status" \
-		--skip "**/trailers-only/unary-ok-but-no-response" \
 		--known-failing @./testing/grpcwebclient-known-failing.txt \
 		-- ./testing/grpcwebclient/bin/grpcwebclient
 

internal/app/connectconformance/server_runner.go

@@ -206,8 +206,10 @@ func runTestCasesForServer(
 				results.setOutcome(name, true, err)
 			case resp.GetError() != nil:
 				results.failed(name, resp.GetError())
-			default:
+			case resp.GetResponse() != nil:
 				results.assert(name, testCase, resp.GetResponse())
+			default:
+				results.setOutcome(name, false, errors.New("client returned a response with neither an error nor result"))
 			}
 			if isReferenceClient && resp.GetResponse() != nil {
 				for _, msg := range resp.GetResponse().Feedback {

testing/grpcwebclient-known-failing.txt

@@ -16,11 +16,6 @@ Duplicate Metadata/**/server-stream/**
 gRPC-Web Trailers/**/trailers-in-body/duplicate-metadata
 gRPC-Web Trailers/**/trailers-in-body/mixed-case
 
-# The gRPC-Web client also fails to verify the grpc-status. If it is missing, it assumes the
-# result is okay, even for a unary RPC where there were zero response messages.
-gRPC-Web Unexpected Responses/**/missing-status
-gRPC-Web Unexpected Responses/**/unary-ok-but-no-response
-
 # The gRPC-Web client also does not use the expected error codes for some error situations.
 # It appears to use "unknown" for nearly (which likely means it's just not classifying them
 # at all, which is certainly a bug).

testing/grpcwebclient/browserscript.ts

@@ -36,15 +36,36 @@ import {
 } from "grpc-web";
 
 // The main entry point into the browser code running in Puppeteer/headless Chrome.
-// This function is invoked by the page.evalulate call in grpcwebclient.
+// This function is invoked by the page.evaluate call in grpcwebclient.
 async function runTestCase(data: number[]): Promise<number[]> {
   const request = ClientCompatRequest.deserializeBinary(new Uint8Array(data));
 
-  const result = await invoke(request);
+  const rpcResult = invoke(request);
+  const timeout = new Promise<ClientResponseResult>((_, reject) => {
+    // Fail if we still don't have a response after 15 seconds
+    // so that user can at least see exactly which test case timed out.
+    setTimeout(() => {
+      reject(new Error("promise never resolved after 15s!"));
+    }, 15*1000);
+  });
+  const result = await Promise.race([rpcResult, timeout]);
 
   return Array.from(result.serializeBinary());
 }
 
+function addErrorListeners() {
+  window.addEventListener("error", function (e) {
+    // @ts-ignore
+    window.log("ERROR: uncaught error in browser: " + e.error.filename + ":" + e.error.lineno + ": " + e.message);
+    return false;
+  })
+  window.addEventListener("unhandledrejection", function (e) {
+    // @ts-ignore
+    window.log("ERROR: unhandled promise failure in browser: " + e.reason);
+    return false;
+  })
+}
+
 function invoke(req: ClientCompatRequest) {
   const client = createClient(req);
   switch (req.getMethod()) {
@@ -117,10 +138,13 @@ function convertGrpcToProtoError(rpcErr: RpcError): ProtoError {
   err.setCode(convertStatusCodeToCode(rpcErr.code));
   err.setMessage(rpcErr.message);
 
-  const value = rpcErr.metadata["grpc-status-details-bin"];
-  if (value) {
-    const status = Status.deserializeBinary(stringToUint8Array(atob(value)));
-    err.setDetailsList(status.getDetailsList());
+  let md = rpcErr.metadata;
+  if (md !== undefined) {
+    const value = md["grpc-status-details-bin"];
+    if (value) {
+      const status = Status.deserializeBinary(stringToUint8Array(atob(value)));
+      err.setDetailsList(status.getDetailsList());
+    }
   }
 
   return err;
@@ -194,6 +218,15 @@ async function unary(
     (err: RpcError, response: UnaryResponse) => {
       if (err !== null) {
         resp.setError(convertGrpcToProtoError(err));
+        let md = err.metadata;
+        if (md !== undefined) {
+          resp.setResponseTrailersList(convertMetadataToHeader(md));
+        }
+        // Ideally, we'd complete the promise from the "end" event. However,
+        // most RPCs that result in an RPC error (as of 3/15/2024, 50 out of
+        // 57 failed RPCs) do not produce an "end" event after the callback
+        // is invoked with an error.
+        res(resp);
       } else {
         const payload = response.getPayload();
         if (payload !== undefined) {
@@ -212,13 +245,22 @@ async function unary(
 
   // Response trailers (i.e. trailing metadata) are sent in the 'status' event
   result.on("status", (status: GrpcWebStatus) => {
+    // One might expect that the "status" event is always delivered (since
+    // consistency would make it much easier to implement interceptors or
+    // decorators, to instrument all RPCs with cross-cutting concerns, like
+    // metrics, logging, etc). But one would be wrong: as of 3/15/2024, there
+    // are 2 cases where the "status" event is never delivered (both cases
+    // are RPC failures).
     const md = status.metadata;
     if (md !== undefined) {
       resp.setResponseTrailersList(convertMetadataToHeader(md));
-      res(resp);
     }
   });
 
+  result.on("end", () => {
+    res(resp);
+  });
+
   return prom;
 }
 
@@ -266,18 +308,31 @@ async function serverStream(
   });
   stream.on("error", (err: RpcError) => {
     resp.setError(convertGrpcToProtoError(err));
+    let md = err.metadata;
+    if (md !== undefined) {
+      resp.setResponseTrailersList(convertMetadataToHeader(md));
+    }
+    // Ideally, we'd complete the promise from the "end" event. However, there
+    // are some RPCs that result in an RPC error (as of 3/15/2024, 3 out of 44
+    // failed RPCs) that do not produce an "end" event after the "error" event.
     res(resp);
   });
 
   // Response trailers (i.e. trailing metadata) are sent in the 'status' event
   stream.on("status", (status: GrpcWebStatus) => {
+    // One might expect that the "status" event is always delivered (since
+    // consistency would make it much easier to implement interceptors or
+    // decorators, to instrument all RPCs with cross-cutting concerns, like
+    // metrics, logging, etc). But one would be wrong: as of 3/15/2024, there
+    // is one case (out of 62 total RPCs) where the "status" event is never
+    // delivered for a streaming call.
     const md = status.metadata;
     if (md !== undefined) {
       resp.setResponseTrailersList(convertMetadataToHeader(md));
     }
   });
 
-  stream.on("end", function () {
+  stream.on("end", () => {
     res(resp);
   });
 
@@ -337,3 +392,5 @@ async function unimplemented(
 
 // @ts-ignore
 window.runTestCase = runTestCase;
+// @ts-ignore
+window.addErrorListeners = addErrorListeners;

testing/grpcwebclient/grpcwebclient.ts

@@ -23,15 +23,31 @@ import {
 } from "./gen/proto/connectrpc/conformance/v1/client_compat_pb.js";
 
 export async function run() {
-  // Launch a browser. For a non-headless browser, pass false
+  // Launch a browser. For a non-headless browser, pass {headless: false}.
+  // Note that the test runner kills the process at the end if it takes too
+  // long to shut down, and puppeteer can't/won't leave browser open after
+  // this script terminates, so the value of non-headless browser is quite
+  // limited. It may be more effective for troubleshooting to instead add
+  // calls to window.log in browserscript.ts (these messages will show up
+  // in the test runner output).
   const browser = await puppeteer.launch({ headless: "new" });
   const page = await browser.newPage();
 
+  await page.exposeFunction("log", (message: string) => {
+    process.stderr.write(message + "\n");
+  })
   await page.addScriptTag({
     type: "application/javascript",
     content: await buildBrowserScript(),
   });
 
+  await page.evaluate(() => {
+    // This allows us log uncaught errors in the browser to this script's
+    // stderr, so they can be seen in the console output when running tests.
+    // @ts-ignore
+    return window.addErrorListeners();
+  });
+
   for await (const next of readReqBuffers(process.stdin)) {
     const req = ClientCompatRequest.deserializeBinary(next);
     const res = new ClientCompatResponse();