Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jest tests for middleware swallow expect errors #270

Closed
iDVB opened this issue Jan 10, 2019 · 5 comments
Closed

Jest tests for middleware swallow expect errors #270

iDVB opened this issue Jan 10, 2019 · 5 comments
Assignees

Comments

@iDVB
Copy link

iDVB commented Jan 10, 2019

I was authoring a custom middy middleware and I implemented Jest tests similar to how you implement them for the core middlewares. (no idea how otherwise to do it) However, many of the tests for core middleware do not fail gracefully...

When a test fails a few things happen that are odd:

  • the Received value is always undefined
  • the handler calls the callback multiple times

Core cors middleware for example:
image
I just caused it to fail by removing the * from the expected Access-Control-Allow-Origin

In order to fix the problem for my own tests which also worked for the core ones, I had to throw when there was an error.

if (err) throw err; 

Example using cors middleware:

test('Access-Control-Allow-Origin header should default to "*"', () => {
    const handler = middy((event, context, cb) => {
      cb(null, {});
    });

    handler.use(cors());

    const event = {
      httpMethod: 'GET',
    };

    handler(event, {}, (err, response) => {
      if (err) throw err;
      expect(response).toEqual({
        headers: {
          'Access-Control-Allow-Origin': '',
        },
      });
    });
  });

I'm new to jest and new to middy. Is there a reason the core middy tests don't fail properly? Or am I just missing something?

@iaindurham
Copy link

When testing async code using callbacks in jest you need to pass in the done method as an argument to the test, then call it once the tests have completed. If you don't do this then the code operation won't pause for the outcome of the test which can cause unexpected results depending on when the code gets run.

See the Jest docs for coding for callbacks.

One thing that the docs don't mention is that if you pass an argument to done in your test, the test will fail with the argument returned as an error message on the console.

So your above example will look like:

  test('Access-Control-Allow-Origin header should default to "*"', (done) => {
    const handler = middy((event, context, cb) => {
      cb(null, {});
    });

    handler.use(cors());

    const event = {
      httpMethod: 'GET',
    };

    handler(event, {}, (err, response) => {
      if (err) return done(err);
      expect(response).toEqual({
        headers: {
          'Access-Control-Allow-Origin': '',
        },
      });
      done();
    });
  });

@iaindurham
Copy link

I just had a quick look at the tests in this repo and noted that they call done -> endTest, so it might be best to call it that in your code if you are going for consistency with the core repo

@iDVB
Copy link
Author

iDVB commented Jan 21, 2019

@iaindurham I'm not sure I follow or that it addresses the issue I'm calling out. The core cors test is synchronous and does not call done.
https://github.com/middyjs/middy/blob/master/src/middlewares/__tests__/cors.js#L19

I'm saying that if I run the jest tests included with this core repo. They work when they PASS.... but they swallow all errors when they fail.

So, if you take this repo's cors test for access-origin and make it fail by removing the *. You should see that it swallows the errors and does not properly report the way jest normally does. See screenshot above.

Seems less like an async testing issue and more of middy swallowing errors.

@iaindurham
Copy link

Yep your right, sorry about that.
When I see issues like poorly returned exception errors in Jest they are often caused by incorrectly wired tests (most common when using promises) so jumped to that conclusion.

@noetix
Copy link
Contributor

noetix commented May 6, 2019

There is so much going on here its hard to understand and explain.

When your response is successful but doesn't match your test, the test fails which throws an error with the assertion failure.

Here is where we go for a ride. Middy will catch the assertion failure as an error in your handler and run its error middlewares.

When middy tries to handle the error it does that by running your response callback but this time with error populated and the response undefined.

Once again your callback containing your test assertions are going to fail.

This is where I think it jumps ship and is no longer synchronous. I can't figure out where exactly, but my sample test passes with an unhandled promise and it fails correctly when I use done().

Test patterns in different middy packages differ on this, some assume synchronous, some use done(), and I guess you could use await. In any case, I fear theres some scenarios passing that shouldn't and regression is likely to be unnoticed.

To get a clean output from Jest I have to treat the scenario as an async test (using done()) and throw on error with if (err) throw err (otherwise the received is undefined making it harder to develop.)

@vladholubiev vladholubiev self-assigned this Sep 5, 2019
vladholubiev added a commit that referenced this issue Sep 5, 2019
fixes #270, fixes 2 tests which were incorrect due callbacks
lmammino pushed a commit that referenced this issue Sep 15, 2019
* test: add expect.hasAssertions before each test

* test: add invoke test helper which promisifies handler

* test: rewrite "packages/cache" tests to async/await

fixes #270

* test: rewrite "packages/do-not-wait-for-empty-event-loop" tests to async/await

fixes #270

* test: rewrite "packages/error-logger" tests to async/await

fixes #270

* test: rewrite "packages/function-shield" tests to async/await

fixes #270

* test: rewrite "packages/http-content-negotiation" tests to async/await

fixes #270

* test: rewrite "packages/http-cors" tests to async/await

fixes #270, fixes 2 tests which were incorrect due callbacks

* test: rewrite "packages/http-error-handler" tests to async/await

fixes #270

* test: rewrite "packages/http-event-normalizer" tests to async/await

fixes #270

* test: rewrite "packages/http-header-normalizer" tests to async/await

fixes #270

* test: rewrite "packages/http-json-body-parser" tests to async/await

fixes #270

* test: rewrite "packages/http-partial-response" tests to async/await

fixes #270

* test: rewrite "packages/http-response-serializer" tests to async/await

fixes #270

* test: rewrite "packages/http-security-header" tests to async/await

fixes #270

* test: rewrite "packages/http-urlencode-body-parser" tests to async/await

fixes #270

* test: rewrite "packages/input-output-logger" tests to async/await

fixes #270

* test: rewrite "packages/s3-key-normalizer" tests to async/await

fixes #270

* test: rewrite "packages/secrets-manager" tests to async/await

fixes #270

* test: rewrite "packages/ssm" tests to async/await

fixes #270

* test: rewrite "packages/validator" tests to async/await

fixes #270

* test: rewrite "packages/warmup" tests to async/await

fixes #270

* Changes to fix tests

* Using es6-promisify for Node6

* Properly added es6-promise as dev dependency to all packages

* importing es6-promisify correctly

* One moar attempt :(

* Version bump
benjifs pushed a commit to benjifs/middy that referenced this issue May 21, 2020
* test: add expect.hasAssertions before each test

* test: add invoke test helper which promisifies handler

* test: rewrite "packages/cache" tests to async/await

fixes middyjs#270

* test: rewrite "packages/do-not-wait-for-empty-event-loop" tests to async/await

fixes middyjs#270

* test: rewrite "packages/error-logger" tests to async/await

fixes middyjs#270

* test: rewrite "packages/function-shield" tests to async/await

fixes middyjs#270

* test: rewrite "packages/http-content-negotiation" tests to async/await

fixes middyjs#270

* test: rewrite "packages/http-cors" tests to async/await

fixes middyjs#270, fixes 2 tests which were incorrect due callbacks

* test: rewrite "packages/http-error-handler" tests to async/await

fixes middyjs#270

* test: rewrite "packages/http-event-normalizer" tests to async/await

fixes middyjs#270

* test: rewrite "packages/http-header-normalizer" tests to async/await

fixes middyjs#270

* test: rewrite "packages/http-json-body-parser" tests to async/await

fixes middyjs#270

* test: rewrite "packages/http-partial-response" tests to async/await

fixes middyjs#270

* test: rewrite "packages/http-response-serializer" tests to async/await

fixes middyjs#270

* test: rewrite "packages/http-security-header" tests to async/await

fixes middyjs#270

* test: rewrite "packages/http-urlencode-body-parser" tests to async/await

fixes middyjs#270

* test: rewrite "packages/input-output-logger" tests to async/await

fixes middyjs#270

* test: rewrite "packages/s3-key-normalizer" tests to async/await

fixes middyjs#270

* test: rewrite "packages/secrets-manager" tests to async/await

fixes middyjs#270

* test: rewrite "packages/ssm" tests to async/await

fixes middyjs#270

* test: rewrite "packages/validator" tests to async/await

fixes middyjs#270

* test: rewrite "packages/warmup" tests to async/await

fixes middyjs#270

* Changes to fix tests

* Using es6-promisify for Node6

* Properly added es6-promise as dev dependency to all packages

* importing es6-promisify correctly

* One moar attempt :(

* Version bump
benjifs pushed a commit to benjifs/middy that referenced this issue May 21, 2020
* test: add expect.hasAssertions before each test

* test: add invoke test helper which promisifies handler

* test: rewrite "packages/cache" tests to async/await

fixes middyjs#270

* test: rewrite "packages/do-not-wait-for-empty-event-loop" tests to async/await

fixes middyjs#270

* test: rewrite "packages/error-logger" tests to async/await

fixes middyjs#270

* test: rewrite "packages/function-shield" tests to async/await

fixes middyjs#270

* test: rewrite "packages/http-content-negotiation" tests to async/await

fixes middyjs#270

* test: rewrite "packages/http-cors" tests to async/await

fixes middyjs#270, fixes 2 tests which were incorrect due callbacks

* test: rewrite "packages/http-error-handler" tests to async/await

fixes middyjs#270

* test: rewrite "packages/http-event-normalizer" tests to async/await

fixes middyjs#270

* test: rewrite "packages/http-header-normalizer" tests to async/await

fixes middyjs#270

* test: rewrite "packages/http-json-body-parser" tests to async/await

fixes middyjs#270

* test: rewrite "packages/http-partial-response" tests to async/await

fixes middyjs#270

* test: rewrite "packages/http-response-serializer" tests to async/await

fixes middyjs#270

* test: rewrite "packages/http-security-header" tests to async/await

fixes middyjs#270

* test: rewrite "packages/http-urlencode-body-parser" tests to async/await

fixes middyjs#270

* test: rewrite "packages/input-output-logger" tests to async/await

fixes middyjs#270

* test: rewrite "packages/s3-key-normalizer" tests to async/await

fixes middyjs#270

* test: rewrite "packages/secrets-manager" tests to async/await

fixes middyjs#270

* test: rewrite "packages/ssm" tests to async/await

fixes middyjs#270

* test: rewrite "packages/validator" tests to async/await

fixes middyjs#270

* test: rewrite "packages/warmup" tests to async/await

fixes middyjs#270

* Changes to fix tests

* Using es6-promisify for Node6

* Properly added es6-promise as dev dependency to all packages

* importing es6-promisify correctly

* One moar attempt :(

* Version bump
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

4 participants