Skip to content
This repository has been archived by the owner on Sep 11, 2024. It is now read-only.

Log unhandled promise rejections in the rageshake #12028

Merged
merged 3 commits into from
Dec 15, 2023
Merged

Log unhandled promise rejections in the rageshake #12028

merged 3 commits into from
Dec 15, 2023

Conversation

uhoreg
Copy link
Member

@uhoreg uhoreg commented Dec 11, 2023

Fixes element-hq/element-web#26639

Checklist

  • Tests written for new code (and old code if feasible)
  • Linter and other CI checks pass
  • Sign-off given on the changes (see CONTRIBUTING.md)

This change is marked as an internal change (Task), so will not be included in the changelog.

@uhoreg uhoreg added the T-Task Refactoring, enabling or disabling functionality, other engineering tasks label Dec 11, 2023
@uhoreg uhoreg requested a review from a team as a code owner December 11, 2023 22:37
@uhoreg uhoreg requested review from dbkr and t3chguy December 11, 2023 22:37
@uhoreg
Copy link
Member Author

uhoreg commented Dec 12, 2023

Not sure what's the best way to test this. Is there a way to check if something got logged?

@t3chguy
Copy link
Member

t3chguy commented Dec 12, 2023

Is there a way to check if something got logged?

You could call IndexedDBLogStore::consume and see whether the line you expect made it?

@uhoreg
Copy link
Member Author

uhoreg commented Dec 13, 2023

It looks like Jest installs its own handler for unhandled rejections, which interferes with the handler that I've installed. I tried writing a test as:

import { init } from "../src/rageshake/rageshake";
import { sleep } from "matrix-js-sdk/src/utils";

describe("rageshake", () => {
    it("should log unhandled promise rejections", async () => {
        await init(false);

        new Promise((resolve, reject) => { reject("foo"); });

        await sleep(2000); // wait for the promise to reject

        const logs = global.mx_rage_logger.flush();

        expect(logs).toMatch(/Unhandled promise rejection: foo/);
    });
});

And get the following output from Jest:

yarn run jest test/Rageshake-test.ts
yarn run v1.22.19
$ /home/hubert/Projects/element/matrix-react-sdk/node_modules/.bin/jest test/Rageshake-test.ts
 FAIL  test/Rageshake-test.ts
  rageshake
    ✕ should log unhandled promise rejections (2006 ms)

  ● rageshake › should log unhandled promise rejections

    thrown: "foo"

      at _getError (node_modules/jest-circus/build/utils.js:432:18)
          at Array.map (<anonymous>)

  ● rageshake › should log unhandled promise rejections

    expect(received).toMatch(expected)

    Expected pattern: /Unhandled promise rejection: foo/
    Received string:  ""

      28 |         const logs = global.mx_rage_logger.flush();
      29 |
    > 30 |         expect(logs).toMatch(/Unhandled promise rejection: foo/);
         |                      ^
      31 |     });
      32 | });
      33 |

      at Object.<anonymous> (test/Rageshake-test.ts:30:22)

Test Suites: 1 failed, 1 total
Tests:       1 failed, 1 total
Snapshots:   0 total
Time:        3.897 s, estimated 4 s
Ran all test suites matching /test\/Rageshake-test.ts/i.
error Command failed with exit code 1.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.

So you can see that Jest handles the rejection.

Any suggestions for how to get around this?

I did try in Element Web, and it does log unhandled promise rejections:

image

@t3chguy
Copy link
Member

t3chguy commented Dec 13, 2023

Any suggestions for how to get around this?

Expect the throw

...
await expect(() => new Promise((resolve, reject) => reject("foo"))).rejects.toThrowError();
const logs = global.mx_rage_logger.flush();
...

(or similar; untested)

Though I guess this may also handle it which means your handler won't fire either. urgh

@uhoreg
Copy link
Member Author

uhoreg commented Dec 14, 2023

Since Jest doesn't want to let me test the code, and none of the other rageshake code has tests either, does anyone mind if I bypass the testing quality gate?

Copy link
Member

@richvdh richvdh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚢

@richvdh richvdh merged commit f0dd12d into matrix-org:develop Dec 15, 2023
1 check passed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
T-Task Refactoring, enabling or disabling functionality, other engineering tasks
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add uncaught exception handler to write Uncaught promise errors in Rageshakes
4 participants