Repository: honeycombio/beeline-nodejs
Pull request #258 (2020-10-21 22:27:43)

add a default for hook.js configure opt's parameter, and some bare bones hooks tests
hooks.js's `configure` shouldn't throw an exception if passed undefined - all other configure methods default to `opts = {}`.  Add a test for the behavior of `configure()` and `configure({})`.

Also add some very contrived and basic tests for parser/propagation hooks (the hooks do no real work at all).  Leave as a comment some cases that are currently failing.  If we're going to actually make an assertion about the _type_ of the return value from the propagation hook, we've got a lot of possible holes.  Same holes exist in the type checking for the return value from `httpTraceParseHook`.



Patch:
diff --git a/lib/propagation/hooks.js b/lib/propagation/hooks.js
index 83d7fc9..044aa7d 100644
--- a/lib/propagation/hooks.js
+++ b/lib/propagation/hooks.js
@@ -8,7 +8,7 @@ let httpTracePropagationHook;
 
 exports.configure = configure;
 
-function configure(opts) {
+function configure(opts = {}) {
   httpTraceParserHook = opts.httpTraceParserHook;
   httpTracePropagationHook = opts.httpTracePropagationHook;
 }
@@ -69,7 +69,7 @@ function headersFromContext(context) {
 
     // if a custom propagation hook returns something truthy but not an object
     // assume configuration error, user returned wrong type
-    if (customSerialized && typeof customSerialized !== "object") {
+    if (typeof customSerialized !== "object") {
       debug(
         "httpTracePropagationHook must return an object containing trace headers as key/value pairs"
       );


Test patch:
diff --git a/lib/propagation/hooks.test.js b/lib/propagation/hooks.test.js
new file mode 100644
index 0000000..64a9598
--- /dev/null
+++ b/lib/propagation/hooks.test.js
@@ -0,0 +1,101 @@
+/* eslint-env jest */
+const { configure, parseFromRequest, headersFromContext } = require("./hooks");
+
+describe("default hooks", () => {
+  test("missing opts", () => {
+    configure();
+    expect(parseFromRequest({})).toBeUndefined();
+    expect(headersFromContext({})).toBeUndefined();
+  });
+
+  test("empty opts", () => {
+    configure({});
+    expect(parseFromRequest({})).toBeUndefined();
+    expect(headersFromContext({})).toBeUndefined();
+  });
+});
+
+describe("parser hook", () => {
+  test("valid fields", () => {
+    function httpTraceParserHook(_req) {
+      return {
+        traceId: "12345",
+        parentSpanId: "67890",
+        dataset: "datasetGoesHere",
+        customContext: { a: 1, b: 2 },
+      };
+    }
+
+    configure({ httpTraceParserHook });
+
+    expect(parseFromRequest({})).toEqual({
+      traceId: "12345",
+      parentSpanId: "67890",
+      dataset: "datasetGoesHere",
+      customContext: { a: 1, b: 2 },
+    });
+  });
+
+  test("'invalid' fields are preserved in the return value", () => {
+    function httpTraceParserHook(_req) {
+      return {
+        invalidTraceIdField: "12345",
+        parentSpanId: "67890",
+        dataset: "datasetGoesHere",
+        customContext: { a: 1, b: 2 },
+      };
+    }
+    configure({ httpTraceParserHook });
+
+    expect(parseFromRequest({})).toEqual({
+      invalidTraceIdField: "12345",
+      parentSpanId: "67890",
+      dataset: "datasetGoesHere",
+      customContext: { a: 1, b: 2 },
+    });
+  });
+
+  test("exception thrown in hook", () => {
+    function httpTraceParserHook(_req) {
+      throw new Error("hello!");
+    }
+
+    configure({ httpTraceParserHook });
+
+    expect(parseFromRequest({})).toBeUndefined();
+  });
+});
+
+describe("propagation hook", () => {
+  test("returning falsey or non-object = undefined", () => {
+    function httpTracePropagationHook(context) {
+      return context;
+    }
+
+    configure({ httpTracePropagationHook });
+
+    expect(headersFromContext(null)).toBeUndefined();
+    expect(headersFromContext(false)).toBeUndefined();
+    expect(headersFromContext("ahem")).toBeUndefined();
+    expect(headersFromContext({ a: 1 })).toEqual({ a: 1 });
+
+    // TODO(toshok) if we're going to have a rule about the return _type_,
+    // these need to be fixed:
+    //
+    // expect(headersFromContext([])).toBeUndefined();
+    // expect(headersFromContext(new Map())).toBeUndefined();
+    // expect(headersFromContext(new Set())).toBeUndefined();
+    // expect(headersFromContext(new ArrayBuffer())).toBeUndefined();
+    // etc.
+  });
+
+  test("exception thrown in hook", () => {
+    function httpTracePropagationHook(_context) {
+      throw new Error("hello!");
+    }
+
+    configure({ httpTracePropagationHook });
+
+    expect(headersFromContext({})).toBeUndefined();
+  });
+});