Let's make sure that we can still check for errors with concise error handling

package.json

@@ -10,9 +10,9 @@
   "scripts": {
     "build": "tsc",
     "lint": "tslint --project ./tsconfig.json -t stylish",
-    "start:dev": "yarn build && node lib/index.js",
+    "start:dev": "yarn build && node --async-stack-traces lib/index.js",
     "test": "ts-mocha --project ./tsconfig.json test/commands/**/*.ts",
-    "test:integration": "NODE_ENV=harness ts-mocha --require test/integration/fixtures.ts --project ./tsconfig.json \"test/integration/**/*Test.ts\"",
+    "test:integration": "NODE_ENV=harness ts-mocha --async-stack-traces --require test/integration/fixtures.ts --project ./tsconfig.json \"test/integration/**/*Test.ts\"",
     "test:manual": "NODE_ENV=harness ts-node test/integration/manualLaunchScript.ts"
   },
   "devDependencies": {

src/index.ts

@@ -27,7 +27,7 @@ import config from "./config";
 import { logMessage } from "./LogProxy";
 import { Healthz } from "./health/healthz";
 import { Mjolnir } from "./Mjolnir";
-import { patchMatrixClientForConciseExceptions } from "./utils";
+import { patchMatrixClient } from "./utils";
 
 config.RUNTIME = {};
 
@@ -52,7 +52,7 @@ if (config.health.healthz.enabled) {
     } else {
         client = new MatrixClient(config.homeserverUrl, config.accessToken, storage);
     }
-    patchMatrixClientForConciseExceptions();
+    patchMatrixClient();
     config.RUNTIME.client = client;
 
     let bot = await Mjolnir.setupMjolnirFromConfig(client);

src/report/ReportManager.ts

@@ -22,7 +22,6 @@ import { JSDOM } from 'jsdom';
 
 import config from "../config";
 import { Mjolnir } from "../Mjolnir";
-import { patchMatrixClientForConciseExceptions } from "../utils";
 
 /// Regexp, used to extract the action label from an action reaction
 /// such as `⚽ Kick user @foobar:localhost from room [kick-user]`.
@@ -876,7 +875,6 @@ class DisplayManager {
                 if (!await action.canExecute(this.owner, report, moderationRoomId)) {
                     continue;
                 }
-                patchMatrixClientForConciseExceptions();
                 await this.owner.mjolnir.client.sendEvent(config.managementRoom, "m.reaction", {
                     "m.relates_to": {
                         "rel_type": "m.annotation",

src/utils.ts

@@ -220,14 +220,15 @@ let isMatrixClientPatchedForConciseExceptions = false;
  * This method configures `MatrixClient` to ensure that methods that may throw
  * instead throws more reasonable insetances of `Error`.
  */
-export function patchMatrixClientForConciseExceptions() {
+function patchMatrixClientForConciseExceptions() {
     if (isMatrixClientPatchedForConciseExceptions) {
         return;
     }
     let originalRequestFn = getRequestFn();
     setRequestFn((params, cb) => {
+        let stack = new Error().stack;
         originalRequestFn(params, function conciseExceptionRequestFn(err, response, resBody) {
-            if (!err && (response.statusCode < 200 || response.statusCode >= 300)) {
+            if (!err && (response?.statusCode < 200 || response?.statusCode >= 300)) {
                 // Normally, converting HTTP Errors into rejections is done by the caller
                 // of `requestFn` within matrix-bot-sdk. However, this always ends up rejecting
                 // with an `IncomingMessage` - exactly what we wish to avoid here.
@@ -277,12 +278,47 @@ export function patchMatrixClientForConciseExceptions() {
                 }
             }
             if ("body" in err) {
-                body = JSON.stringify((err as any).body);
+                body = (err as any).body;
             }
             let error = new Error(`Error during MatrixClient request ${method} ${path}: ${err.statusCode} ${err.statusMessage} -- ${body}`);
+            error.stack = stack;
+            if (body) {
+                // Calling code may use `body` to check for errors, so let's
+                // make sure that we're providing it.
+                try {
+                    body = JSON.parse(body);
+                } catch (ex) {
+                    // Not JSON.
+                }
+                // Define the property but don't make it visible during logging.
+                Object.defineProperty(error, "body", {
+                    value: body,
+                    enumerable: false,
+                });
+            }
+            // Calling code may use `statusCode` to check for errors, so let's
+            // make sure that we're providing it.
+            if ("statusCode" in err) {
+                Object.defineProperty(error, "statusCode", {
+                    value: err.statusCode,
+                    enumerable: false,
+                });
+            }
             return cb(error, response, resBody);
         })
     });
     isMatrixClientPatchedForConciseExceptions = true;
 }
 
+/**
+ * Perform any patching deemed necessary to MatrixClient.
+ */
+export function patchMatrixClient() {
+    // Note that the order of patches is meaningful.
+    //
+    // - `patchMatrixClientForConciseExceptions` converts all `IncomingMessage`
+    //   errors into instances of `Error` handled as errors;
+    // - `patchMatrixClientForRetry` expects that all errors are returned as
+    //   errors.
+    patchMatrixClientForConciseExceptions();
+}

test/integration/abuseReportTest.ts

@@ -21,7 +21,7 @@ const REPORT_NOTICE_REGEXPS = {
 
 describe("Test: Reporting abuse", async () => {
     it('Mjölnir intercepts abuse reports', async function() {
-        this.timeout(10000);
+        this.timeout(60000);
 
         // Listen for any notices that show up.
         let notices = [];
@@ -216,7 +216,7 @@ describe("Test: Reporting abuse", async () => {
         }
     });
     it('The redact action works', async function() {
-        this.timeout(10000);
+        this.timeout(60000);
 
         // Listen for any notices that show up.
         let notices = [];

test/integration/clientHelper.ts

@@ -1,6 +1,6 @@
 import axios from "axios";
 import { HmacSHA1 } from "crypto-js";
-import { extractRequestError, LogService, MatrixClient, MemoryStorageProvider, PantalaimonClient } from "matrix-bot-sdk";
+import { LogService, MatrixClient, MemoryStorageProvider, PantalaimonClient } from "matrix-bot-sdk";
 import config from "../../src/config";
 
 /**

test/integration/commands/redactCommandTest.ts

@@ -8,7 +8,7 @@ import { onReactionTo } from "./commandUtils";
 
  describe("Test: The redaction command", function () {
     it('Mjölnir redacts all of the events sent by a spammer when instructed to by giving their id and a room id.', async function() {
-        this.timeout(20000);
+        this.timeout(60000);
         // Create a few users and a room.
         let badUser = await newTestUser(false, "spammer-needs-redacting");
         let badUserId = await badUser.getUserId();
@@ -52,7 +52,7 @@ import { onReactionTo } from "./commandUtils";
         });
     })
     it('Mjölnir redacts all of the events sent by a spammer when instructed to by giving their id in multiple rooms.', async function() {
-        this.timeout(20000);
+        this.timeout(60000);
         // Create a few users and a room.
         let badUser = await newTestUser(false, "spammer-needs-redacting");
         let badUserId = await badUser.getUserId();
@@ -101,7 +101,7 @@ import { onReactionTo } from "./commandUtils";
         });
     });
     it("Redacts a single event when instructed to.", async function () {
-        this.timeout(20000);
+        this.timeout(60000);
         // Create a few users and a room.
         let badUser = await newTestUser(false, "spammer-needs-redacting");
         const mjolnir = config.RUNTIME.client!

test/integration/mjolnirSetupUtils.ts

@@ -24,7 +24,7 @@ import {
 import { Mjolnir}  from '../../src/Mjolnir';
 import config from "../../src/config";
 import { registerUser } from "./clientHelper";
-import { patchMatrixClientForConciseExceptions } from "../../src/utils";
+import { patchMatrixClient } from "../../src/utils";
 
 /**
  * Ensures that a room exists with the alias, if it does not exist we create it.
@@ -33,8 +33,9 @@ import { patchMatrixClientForConciseExceptions } from "../../src/utils";
  * @returns The room ID of the aliased room.
  */
 export async function ensureAliasedRoomExists(client: MatrixClient, alias: string): Promise<string> {
-    return await client.resolveRoom(alias)
+    try {
-    .catch(async e => {
+        return await client.resolveRoom(alias);
+    } catch (e) {
         if (e?.body?.errcode === 'M_NOT_FOUND') {
             console.info(`${alias} hasn't been created yet, so we're making it now.`)
             let roomId = await client.createRoom({
@@ -44,7 +45,7 @@ export async function ensureAliasedRoomExists(client: MatrixClient, alias: strin
             return roomId
         }
         throw e;
-    });
+    }
 }
 
 async function configureMjolnir() {
@@ -81,7 +82,7 @@ export async function makeMjolnir(): Promise<Mjolnir> {
     LogService.info("test/mjolnirSetupUtils", "Starting bot...");
     const pantalaimon = new PantalaimonClient(config.homeserverUrl, new MemoryStorageProvider());
     const client = await pantalaimon.createClientWithCredentials(config.pantalaimon.username, config.pantalaimon.password);
-    patchMatrixClientForConciseExceptions();
+    patchMatrixClient();
     await ensureAliasedRoomExists(client, config.managementRoom);
     let mjolnir = await Mjolnir.setupMjolnirFromConfig(client);
     globalClient = client;

test/integration/timelinePaginationTest.ts

@@ -8,7 +8,7 @@ import { getMessagesByUserIn } from "../../src/utils";
  */
 describe("Test: timeline pagination", function () {
     it('does not paginate across the entire room history while backfilling.', async function() {
-        this.timeout(20000);
+        this.timeout(60000);
         // Create a few users and a room.
         let badUser = await newTestUser(false, "spammer");
         let badUserId = await badUser.getUserId();
@@ -38,7 +38,7 @@ describe("Test: timeline pagination", function () {
         assert.equal(eventCount, 7, "There shouldn't be any more events (1 member event and 6 messages), and they should all be from the same account.");
     })
     it('does not call the callback with an empty array when there are no relevant events', async function() {
-        this.timeout(20000);
+        this.timeout(60000);
         let badUser = await newTestUser(false, "spammer");
         let badUserId = await badUser.getUserId();
         let moderator = await newTestUser(false, "moderator");
@@ -53,7 +53,7 @@ describe("Test: timeline pagination", function () {
         assert.equal(cbCount, 0, "The callback should never get called");
     })
     it("The limit provided is respected", async function() {
-        this.timeout(20000);
+        this.timeout(60000);
         let badUser = await newTestUser(false, "spammer");
         let badUserId = await badUser.getUserId();
         let moderator = await newTestUser(false, "moderator");
@@ -82,7 +82,7 @@ describe("Test: timeline pagination", function () {
         assert.equal(cbCount, 1, "The callback should be called once with events matching the glob.");
     });
     it("Gives the events to the callback ordered by youngest first (even more important when the limit is reached halfway through a chunk).", async function() {
-        this.timeout(20000);
+        this.timeout(60000);
         let moderator = await newTestUser(false, "moderator");
         let moderatorId = await moderator.getUserId();
         let targetRoom = await moderator.createRoom();