chore(tests): Convert to typescript and use Jest #188

wolfy1339 · 2020-08-06T17:40:37Z

Fixes #187

test/unit/event-handler-wrap-error-handler-test.ts

oscard0m · 2020-08-06T17:58:47Z

Would you agree on putting in place ESlint rules for jest? What about the recommended preset?

oscard0m · 2020-08-06T17:59:32Z

@wolfy1339 if you need help or want to split some tests together (as branch from your branch) let me know :)

wolfy1339 · 2020-08-07T03:15:28Z

Would you agree on putting in place ESlint rules for jest? What about the recommended preset?

ESLint isn't used (at least exposed to the user), since Prettier is used

Found this little bug when doing #188

Found this little bug when doing #188 Co-authored-by: OscarDOM <dominguez.celada@gmail.com>

wolfy1339 · 2020-08-07T17:35:16Z

I need some help. I've got things mostly done here.

The tests in test/integration/event-handler-test.ts are failing due to async timeout.
There's also test/integration/middleware-test.ts, test/unit/middleware-test.ts, test/integration/server-test.ts that need to be looked at, it's mostly just type problems.

Relevant logs:

FAIL  test/unit/middleware-test.ts
  ● Test suite failed to run

    test/unit/middleware-test.ts:12:21 - error TS2345: Argument of type '{ method: string; url: string; }' is not assignable to parameter of type 'IncomingMessage'.
      Type '{ method: string; url: string; }' is missing the following properties from type 'IncomingMessage': aborted, httpVersion, httpVersionMajor, httpVersionMinor, and 45 more.

    12   middleware(state, { method: "POST", url: "/foo" }, {}, next);
                           ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

 FAIL  test/integration/middleware-test.ts
  ● Test suite failed to run

    test/integration/middleware-test.ts:32:14 - error TS2345: Argument of type '{ method: string; headers: { "x-github-delivery": string; "x-github-event": string; "x-hub-signature": string; }; url: string; addListener(event: string | symbol, listener: (...args: any[]) => void): EventEmitter; ... 13 more ...; eventNames(): (string | symbol)[]; }' is not assignable to parameter of type 'IncomingMessage'.
      Type '{ method: string; headers: { "x-github-delivery": string; "x-github-event": string; "x-hub-signature": string; }; url: string; addListener(event: string | symbol, listener: (...args: any[]) => void): EventEmitter; ... 13 more ...; eventNames(): (string | symbol)[]; }' is missing the following properties from type 'IncomingMessage': aborted, httpVersion, httpVersionMajor, httpVersionMinor, and 29 more.

    32   middleware(requestMock, responseMock).then(() => {
                    ~~~~~~~~~~~
    test/integration/middleware-test.ts:50:17 - error TS2554: Expected 1 arguments, but got 0.

    50     end: simple.spy(),
                       ~~~~~

      node_modules/@types/simple-mock/index.d.ts:42:12
        42     spy<T>(fn: Fn<T>): Spy<T>;
                      ~~~~~~~~~
        An argument for 'fn' was not provided.
    test/integration/middleware-test.ts:55:14 - error TS2345: Argument of type '{ method: string; headers: { "x-github-delivery": string; "x-github-event": string; "x-hub-signature": string; }; url: string; addListener(event: string | symbol, listener: (...args: any[]) => void): EventEmitter; ... 13 more ...; eventNames(): (string | symbol)[]; }' is not assignable to parameter of type 'IncomingMessage'.
      Type '{ method: string; headers: { "x-github-delivery": string; "x-github-event": string; "x-hub-signature": string; }; url: string; addListener(event: string | symbol, listener: (...args: any[]) => void): EventEmitter; ... 13 more ...; eventNames(): (string | symbol)[]; }' is missing the following properties from type 'IncomingMessage': aborted, httpVersion, httpVersionMajor, httpVersionMinor, and 29 more.

    55   middleware(requestMock, responseMock).then(() => {
                    ~~~~~~~~~~~

 FAIL  test/integration/server-test.ts
  ● Test suite failed to run

    test/integration/server-test.ts:101:11 - error TS2339: Property 'body' does not exist on type 'IncomingMessage'.

    101       req.body = JSON.parse(Buffer.concat(dataChunks).toString());
                  ~~~~
    test/integration/server-test.ts:147:19 - error TS2345: Argument of type 'Spy<unknown>' is not assignable to parameter of type '(event: WebhookEventHandlerError) => void | Promise<void>'.
      Type 'unknown' is not assignable to type 'void | Promise<void>'.
        Type '{}' is missing the following properties from type 'Promise<void>': then, catch, [Symbol.toStringTag], finally

    147   api.on("error", errorHandler);
                          ~~~~~~~~~~~~
    test/integration/server-test.ts:181:31 - error TS2554: Expected 1 arguments, but got 0.

    181   const errorHandler = simple.spy();
                                      ~~~~~

      node_modules/@types/simple-mock/index.d.ts:42:12
        42     spy<T>(fn: Fn<T>): Spy<T>;
                      ~~~~~~~~~
        An argument for 'fn' was not provided.
    test/integration/server-test.ts:182:19 - error TS2345: Argument of type 'Spy<unknown>' is not assignable to parameter of type '(event: WebhookEventHandlerError) => void | Promise<void>'.

    182   api.on("error", errorHandler);
                          ~~~~~~~~~~~~

 FAIL  test/integration/event-handler-test.ts (19.016 s)
  ● events

    : Timeout - Async callback was not invoked within the 5000 ms timeout specified by jest.setTimeout.Timeout - Async callback was not invoked within the 5000 ms timeout specified by jest.setTimeout.Error:

      3 | import installationCreatedPayload from "../fixtures/installation-created-payload.json";
      4 | 
    > 5 | test("events", (t) => {
        | ^
      6 |   // expect.assertions(7);
      7 | 
      8 |   const eventHandler = createEventHandler({});

      at new Spec (node_modules/jest-jasmine2/build/jasmine/Spec.js:116:22)
      at Object.<anonymous> (test/integration/event-handler-test.ts:5:1)

  ● options.transform

    : Timeout - Async callback was not invoked within the 5000 ms timeout specified by jest.setTimeout.Timeout - Async callback was not invoked within the 5000 ms timeout specified by jest.setTimeout.Error:

      101 | });
      102 | 
    > 103 | test("options.transform", (t) => {
          | ^
      104 |   expect.assertions(2);
      105 | 
      106 |   const eventHandler = createEventHandler({

      at new Spec (node_modules/jest-jasmine2/build/jasmine/Spec.js:116:22)
      at Object.<anonymous> (test/integration/event-handler-test.ts:103:1)

  ● multiple errors in same event handler

    : Timeout - Async callback was not invoked within the 5000 ms timeout specified by jest.setTimeout.Timeout - Async callback was not invoked within the 5000 ms timeout specified by jest.setTimeout.Error:

      141 | });
      142 | 
    > 143 | test("multiple errors in same event handler", (t) => {
          | ^
      144 |   expect.assertions(2);
      145 | 
      146 |   const eventHandler = createEventHandler({});

      at new Spec (node_modules/jest-jasmine2/build/jasmine/Spec.js:116:22)
      at Object.<anonymous> (test/integration/event-handler-test.ts:143:1)

wolfy1339 · 2020-08-08T03:11:59Z

@dominguezcelada Care to help me on this? I noted the errors I'm getting in my previous comment

oscard0m · 2020-08-08T09:15:37Z

The tests in test/integration/event-handler-test.ts are failing due to async timeout.

I opened a PR to this branch which I think is solving the issue with timeouts: #195

If you think this is the solution, feel free to merge it in your branch :)

oscard0m · 2020-08-08T10:40:19Z

There's also test/integration/middleware-test.ts,

#196

test/unit/middleware-test.ts,

#197

test/integration/server-test.ts that need to be looked at, it's mostly just type problems.

#198 WIP

Once we finish this, if this is the right approach to follow, what do you think about getting rid of simple-mock library? I think the usage we are giving to this library is already covered by jest itself

oscard0m · 2020-08-08T10:44:30Z

btw, before doing merge, I think we should make the first commit (or the resulting squashed one to be test(<scope>): commit message as type instead of chore(<scope>): commit message 😎

oscard0m · 2020-08-09T11:05:15Z

test/integration/server-test.ts that need to be looked at, it's mostly just type problems.

#198 WIP

#198 ✅ @wolfy1339

After tsconfig.json changes in master with strict: true new TypeScript errors were introduced 😢 . Once we validate my PR in here I can jump to fix them if you want

oscard0m · 2020-08-22T19:42:14Z

Hey @wolfy1339 it seems we were overlaping on fixing the changes. If there's something I can help or we can split / pair-program let me know! :)

wolfy1339 · 2020-08-22T19:44:18Z

Just leave a comment as to what file/what issue you're working on, that should be just fine

oscard0m · 2020-08-22T20:06:39Z

Just leave a comment as to what file/what issue you're working on, that should be just fine

Getting event-handler-test.ts errors @wolfy1339

oscard0m · 2020-08-22T20:21:37Z

Just leave a comment as to what file/what issue you're working on, that should be just fine

Getting event-handler-test.ts errors

Getting sign-test.ts errors @wolfy1339

oscard0m · 2020-08-22T20:43:29Z

Just leave a comment as to what file/what issue you're working on, that should be just fine

Getting event-handler-test.ts errors

Getting sign-test.ts errors @wolfy1339

Getting server-test.ts errors @wolfy1339

oscard0m · 2020-08-23T00:18:34Z

We only need to fix the coverage for middleware-test.ts and we will be ready to go 👏 @wolfy1339

wolfy1339 · 2020-08-23T04:24:26Z

@dominguezcelada

According to the coverage report, it seems that the lines that aren't being used are related to timeouts, which was added in dfdfc46

From what I can understand of the code, a timeout needs to be simulated and doing so using @sinonjs/fake-timers.
One is request with no errors and just a timeout, the other an error is thrown and it times out.

@gr2m Any ideas how to test that

oscard0m · 2020-08-23T07:51:14Z

@dominguezcelada

According to the coverage report, it seems that the lines that aren't being used are related to timeouts, which was added in dfdfc46

From what I can understand of the code, a timeout needs to be simulated and doing so using @sinonjs/fake-timers.
One is request with no errors and just a timeout, the other an error is thrown and it times out.

@gr2m Any ideas how to test that

I just applied the test coverage. I'm using Jest's Timer Mocks utility. Is that ok @gr2m @wolfy1339 ?

Fix some types Replace some remaining tap functions Ignore some TypeScript type errors, they are there on purpose to test if the function throws an error

…ed to run are executed

…mes(1)

… test to wait until all expect

- Pull in the relevant types package for `@sinonjs/fake-timers` - Convert those tests from tap to Jest - Add some Typescript types to those tests

…ndler-test (#212)

… a timeout (#215)

Adds back the existing error message that was removed

gr2m · 2020-08-25T06:52:09Z

As sorry, I thought you meant the error was on master.

I got a pull request merged into aggregate-error: sindresorhus/aggregate-error#13. But before this was merged, I added new the type definitions directly into @octokit/webhooks. With the latest change, I upgraded to latest aggregate-error and removed to temporary types that I added, as they are now obsolete.

I can look into the error on wolfy1339-patch-1 tomorrow

oscard0m · 2020-08-25T08:11:47Z

As sorry, I thought you meant the error was on master.

I got a pull request merged into aggregate-error: sindresorhus/aggregate-error#13. But before this was merged, I added new the type definitions directly into @octokit/webhooks. With the latest change, I upgraded to latest aggregate-error and removed to temporary types that I added, as they are now obsolete.

I can look into the error on wolfy1339-patch-1 tomorrow

@wolfy1339 @gr2m Solved 10386fa (#188) by upgrading aggregate-error package to latest version with @gr2m 's fix (sindresorhus/aggregate-error#13)

oscard0m · 2020-08-25T08:17:34Z

I think we are ready to go now 💪 🍾

gr2m · 2020-08-25T17:34:18Z

src/sign/index.ts

@@ -1,6 +1,7 @@
 import { createHmac } from "crypto";

-export function sign(secret: string, payload: string | object): string {
+export function sign(secret?: string, payload?: string | object): string {


Can you revert this change?

Can you revert this change?

@gr2m without this change we are not able to give coverage to this

webhooks.js/src/sign/index.ts

Lines 4 to 6 in d4bd29b

if (!secret || !payload) {

throw new TypeError("secret & payload required");

}

Maybe you prefer this approach on for tests so we give coverage but we bypass the TypeScript check? #237

gr2m

Outstanding work @wolfy1339 & @dominguezcelada 🥇🥇🎊

github-actions · 2020-09-19T18:22:34Z

🎉 This PR is included in version 7.11.3 🎉

The release is available on:

Your semantic-release bot 📦🚀

wolfy1339 added typescript Relevant to TypeScript users only Type: Maintenance Any dependency, housekeeping, and clean up Issue or PR labels Aug 6, 2020

wolfy1339 commented Aug 6, 2020

View reviewed changes

test/unit/event-handler-wrap-error-handler-test.ts Outdated Show resolved Hide resolved

test/unit/event-handler-wrap-error-handler-test.ts Outdated Show resolved Hide resolved

gr2m mentioned this pull request Aug 6, 2020

Rewrite tests in TypeScript and Jest #187

Closed

wolfy1339 force-pushed the wolfy1339-patch-1 branch 2 times, most recently from e48290c to 7a6ce05 Compare August 7, 2020 04:40

wolfy1339 added a commit that referenced this pull request Aug 7, 2020

fix(typescript): Remove file extension from import

2b5935e

Found this little bug when doing #188

wolfy1339 mentioned this pull request Aug 7, 2020

fix(typescript): Remove file extension from import #193

Merged

oscard0m added a commit that referenced this pull request Aug 7, 2020

fix(typescript): Remove file extension from import (#193)

459f5da

Found this little bug when doing #188 Co-authored-by: OscarDOM <dominguez.celada@gmail.com>

wolfy1339 force-pushed the wolfy1339-patch-1 branch from 63eacc2 to aab4b4f Compare August 7, 2020 16:23

wolfy1339 force-pushed the wolfy1339-patch-1 branch from 6022d1d to 50322a1 Compare August 9, 2020 01:28

wolfy1339 force-pushed the wolfy1339-patch-1 branch from b3f07a9 to 1cd9000 Compare August 22, 2020 19:21

wolfy1339 and others added 15 commits August 25, 2020 02:49

fix(tests): Fix various problems in the tests

925a2ab

Fix some types Replace some remaining tap functions Ignore some TypeScript type errors, they are there on purpose to test if the function throws an error

test(integration/middlewares): fix mocks to make tests pass

6806c2f

test(middleware): add check to assure the number of assertions expect…

1005067

…ed to run are executed

test(middlewares): fix typing of unit test for middlewares

df6345f

♻️ refactor(jest): use toHaveBeenCalled instead of toHaveBeenCalledTi…

053fb62

…mes(1)

🚨 test(event-handler-test): add missing done() callback to make async…

ea5ce5f

… test to wait until all expect

tests: Set node environment in Jest config

29721df

test(server-test): mock errorHandler using jest.fn() (#198)

9029869

tests: Fix tests after #190

0439419

- Pull in the relevant types package for `@sinonjs/fake-timers` - Convert those tests from tap to Jest - Add some Typescript types to those tests

fix(typescript): Fix some type errors in the tests

a21da42

test(types): add missing types to variables and paramters on event-ha…

034f582

…ndler-test (#212)

Fix sign test type compilation error (#213)

0ca75c0

test(server-test): fix typescript errors (#214)

ec2a383

test(middleware-test): add test coverage for middleware when there is…

90eaa87

… a timeout (#215)

Revert 2b7c0bb and instead add a ts-ignore statement

3825b15

Adds back the existing error message that was removed

wolfy1339 force-pushed the wolfy1339-patch-1 branch from 3e77959 to 3825b15 Compare August 25, 2020 06:49

build(aggregate-error): upgrade version to support Custom Errors (#233)

10386fa

test(sign-test): restore 100% coverage for sign-test (#234)

fbc8196

gr2m requested changes Aug 25, 2020

View reviewed changes

oscard0m requested a review from gr2m August 25, 2020 19:18

fix(sign): make sign() parameters non-optional (#237)

30f851c

gr2m approved these changes Aug 25, 2020

View reviewed changes

gr2m merged commit 7e8defa into master Aug 25, 2020

gr2m deleted the wolfy1339-patch-1 branch August 25, 2020 19:43

github-actions bot added the released label Sep 19, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore(tests): Convert to typescript and use Jest #188

chore(tests): Convert to typescript and use Jest #188

wolfy1339 commented Aug 6, 2020

oscard0m commented Aug 6, 2020

oscard0m commented Aug 6, 2020

wolfy1339 commented Aug 7, 2020

wolfy1339 commented Aug 7, 2020

wolfy1339 commented Aug 8, 2020

oscard0m commented Aug 8, 2020

oscard0m commented Aug 8, 2020 •

edited

Loading

oscard0m commented Aug 8, 2020 •

edited

Loading

oscard0m commented Aug 9, 2020 •

edited

Loading

oscard0m commented Aug 22, 2020

wolfy1339 commented Aug 22, 2020 via email

oscard0m commented Aug 22, 2020 •

edited

Loading

oscard0m commented Aug 22, 2020 •

edited

Loading

oscard0m commented Aug 22, 2020

oscard0m commented Aug 23, 2020

wolfy1339 commented Aug 23, 2020 •

edited

Loading

oscard0m commented Aug 23, 2020

gr2m commented Aug 25, 2020

oscard0m commented Aug 25, 2020

oscard0m commented Aug 25, 2020 •

edited

Loading

gr2m Aug 25, 2020

oscard0m Aug 25, 2020

gr2m left a comment

github-actions bot commented Sep 19, 2020

	if (!secret \|\| !payload) {
	throw new TypeError("secret & payload required");
	}

chore(tests): Convert to typescript and use Jest #188

chore(tests): Convert to typescript and use Jest #188

Conversation

wolfy1339 commented Aug 6, 2020

oscard0m commented Aug 6, 2020

oscard0m commented Aug 6, 2020

wolfy1339 commented Aug 7, 2020

wolfy1339 commented Aug 7, 2020

wolfy1339 commented Aug 8, 2020

oscard0m commented Aug 8, 2020

oscard0m commented Aug 8, 2020 • edited Loading

oscard0m commented Aug 8, 2020 • edited Loading

oscard0m commented Aug 9, 2020 • edited Loading

oscard0m commented Aug 22, 2020

wolfy1339 commented Aug 22, 2020 via email

oscard0m commented Aug 22, 2020 • edited Loading

oscard0m commented Aug 22, 2020 • edited Loading

oscard0m commented Aug 22, 2020

oscard0m commented Aug 23, 2020

wolfy1339 commented Aug 23, 2020 • edited Loading

oscard0m commented Aug 23, 2020

gr2m commented Aug 25, 2020

oscard0m commented Aug 25, 2020

oscard0m commented Aug 25, 2020 • edited Loading

gr2m Aug 25, 2020

Choose a reason for hiding this comment

oscard0m Aug 25, 2020

Choose a reason for hiding this comment

gr2m left a comment

Choose a reason for hiding this comment

github-actions bot commented Sep 19, 2020

oscard0m commented Aug 8, 2020 •

edited

Loading

oscard0m commented Aug 8, 2020 •

edited

Loading

oscard0m commented Aug 9, 2020 •

edited

Loading

oscard0m commented Aug 22, 2020 •

edited

Loading

oscard0m commented Aug 22, 2020 •

edited

Loading

wolfy1339 commented Aug 23, 2020 •

edited

Loading

oscard0m commented Aug 25, 2020 •

edited

Loading