v5.5.0 broke mocks with jest.fn

[JaroVDH]

Version 5.5.0 broke existing mocks using jest.fn.
The "callback" parameter is nolonger forwarded to the mock function.

These changes seem to have broken it:
aed50d1#diff-e727e4bdf3657fd1d798edcd6b099d6e092f8573cba266154583a746bba0f346R119

replace() is not called with cb.

Minimal repro test with Jest:

const AWSMock = require('aws-sdk-mock');
const AWS = require('aws-sdk');

// Works
test('callback should be a function', async () => {
	AWSMock.mock('DynamoDB', 'getItem', (params, callback) => {
		expect(typeof callback).toBe('function');
	});

	const dynamodb = new AWS.DynamoDB({apiVersion: '2012-08-10'});

	await dynamodb.getItem({ TableName: '', Key: {} }).promise();
});

// Fails, callback is undefined
test('callback should be a function with jest.fn', async () => {
	AWSMock.mock('DynamoDB', 'getItem', jest.fn((params, callback) => {
		expect(typeof callback).toBe('function');
	}));

	const dynamodb = new AWS.DynamoDB({apiVersion: '2012-08-10'});

	await dynamodb.getItem({ TableName: '', Key: {} }).promise();
});

Unless the intent is to only support direct returns from mock functions rather than callback calls.
I'd classify that as a major release though, not a minor 😅

I'm also not sure how you could easily support both approaches.

[JaroVDH]

One way of addressing this could be to wrap cb() and check if it has been called.
If not, continue as normal.
For example:

let cbCalled = false;

result = replace(params, (...args) => {
  cbCalled = true;
  cb(args...);
});

if(!cbCalled) {
  if (typeof result.then === 'function') {
    result.then(val => cb(undefined, val), err => cb(err));
  } else {
    cb(undefined, result);
  }
}

Still has the issue of exceptions, though.

[thomaux]

Good catch, we didn't account for the case where the jest mock was passed an implementation of its own. I need some time to investigate how to best tackle this case while still providing the benefits of 5.5.0

[loopingz]

Also if used with a void method like https://docs.aws.amazon.com/AWSJavaScriptSDK/latest/AWS/EC2.html#describeVpcs-property

await ec2.describeVpcs().promise();
// or
sts.getCallerIdentity().promise()

// When mocked it fails because params is the callback and cb is empty
// It should have something like
if (cb === undefined) {
  cb = params;
  params = {};
}

[thomaux]

Thanks for the feedback everyone, picking this up next week.

[eranelbaz]

Any updates?

[thomaux]

I've been looking over this, but I don't believe there's an easy way to fix the issues caused by this change. It's also rather invasive that the library automatically wraps, but even if we would move this wrapping behind a configuration flag, it would still cause issues when active.

So at this point I would opt to rollback this change, what's your thought @nelsonic ?

[nelsonic]

@thomaux I don't think we can unpublish the latest (5.5.0)version ... 😕
https://blog.npmjs.org/post/141905368000/changes-to-npms-unpublish-policy

We could roll-back by publishing a new version e.g. 5.5.1 but that feels "messy". 🙄
What about integrating the code as suggested by @JaroVDH above #256 (comment) ? 💭

[thomaux]

@nelsonic like @JaroVDH mentioned, integrating the code like that would not handle exceptions..

I believe the culprit is line 126:
const result = replace(params);

Passing the callback there, like:
const result = replace(params, cb);

Should correctly invoke the replacement function. I'll spend some time on a solution around this.

Agreed that we don't want to (can't) unpublish.

A temporary solution would be to move this mapping behind a config flag, and release that as 5.5.1.
awsSdkMock.enableTestStubWrapping()

[nelsonic]

I have a strong preference for the longer term solution. Especially if the people affected (anyone using Jest) can help verify that it works. 🤞

[loopingz]

It is not only Jest that is impacted, I'm using it with sinon and had to rewrite most of my mocked function callsFake(mockedFunction) but got stuck with the default empty parameters bug

[nelsonic]

@loopingz can you not just use the previous versions of the package instead of rewriting? 🤷🏼‍♂️
(sorry for the inconvenience…)

[loopingz]

@nelsonic yes i did revert at first, then I was okay to try to accomadate your changes, so i started a branch to do so, but got into the empty parameters bug, so I kept the 5.4.0 but now dependabot is bugging me :)

loopingz/webda.io@8388b15

[nelsonic]

Gotcha. Thanks @dependabot 💭

[thomaux]

Allright, based on the input of @JaroVDH and @loopingz (many thanks), I think I have been able to fix the issues reported above. The code can be found here: https://github.com/thomaux/aws-sdk-mock/blob/fix-256/index.js#L124

The current code will not handle this case (a replacement function that only defines the callback):

const jestMock = jest.fn((cb) => cb(null, 'item'));
awsMock.mock('DynamoDB', 'getItem', jestMock);

Do we want to support this?

[thomaux]

I opened a PR for this, I went ahead and did add a clause to the code to also handle the following case:

const jestMock = jest.fn((cb) => cb(null, 'item'));
awsMock.mock('DynamoDB', 'getItem', jestMock);

[nelsonic]

aws-sdk-mock@5.5.1 includes the fix by @thomaux #257 📦
Please test and confirm that it's working. Thanks. 🤞

[JaroVDH]

Back from vacation and ran our tests against 5.5.1, no more failures 👍

[mjdickinson]

I preferred the 5.5.0 behavior since we are mocking with promise returning functions. We don't need, or expect, a callback function to be passed to replace. In fact, I was pleased to be able to replace every
expect(serviceFunction).toHaveBeenCalledWith(expectedParams, expect.any(Function))
in our tests with
expect(serviceFunction).toHaveBeenCalledWith(expectedParams)

Is there some way to support both mocking styles? Maybe pass an extra parameter to mock() telling it to not pass a callback? Or add an alternative such as mockWithoutCallback()?

[thomaux]

@mjdickinson The 5.5.0 behaviour is still supported. The SDK methods are always called with a callback, even if you use .promise() (it will just add a default callback passing errors to reject and results to resolve).

5.5.0 introduced this option, which in my opinion offers cleaner tests:

// Given
const putMock = jest.fn();
mockAWS('DynamoDB.DocumentClient', 'put', putMock);

// When
await documentClient.put(item).promise();

// Then
expect(putMock).toHaveBeenCalledWith(item);

[mjdickinson]

Prior to 5.5.0 our tests were pretty clean:

// Given
const listBuckets = jest.fn(() => Promise.resolve({ Buckets: [] }));
awsMock.mock('S3', 'listBuckets', listBuckets);

// When
await new aws.S3().listBuckets({}).promise();

// Then
expect(listBuckets).toHaveBeenCalledWith({}, expect.any(Function));

5.5.0 allowed us to remove the expect.any(Function) from the expectation, which was pleasing because it feels like accidental noise. With 5.5.1 we have to add it back again.

I understand that 5.5.0 broke callback style tests. Indeed, it broke all of our tests too. I was just wondering if there is any interest in being able to control whether a callback is passed to the mock or not? We would certainly appreciate it the interest of cleaner promise style tests.

// Given
const listBuckets = jest.fn(() => Promise.resolve({ Buckets: [] }));
awsMock.mockNoCallback('S3', 'listBuckets', listBuckets);

// When
await new aws.S3().listBuckets({}).promise();

// Then
expect(listBuckets).toHaveBeenCalledWith({});

For the moment we're sticking with 5.5.0 because we prefer the tests without the extra expect.any(Function)s.

[thomaux]

Thanks for the details @mjdickinson, it was my understanding the behaviour you are seeking was still supported. I'll reproduce this example and see what's going wrong in 5.5.1.

For the record, the whole reason I initially suggested the changes for 5.5.0 was to make it easier to test using Jest. I didn't want to manually have to wrap my Jest Mocks before passing them to the library.

Just out of curiosity, does the following approach work (notice the change in how I defined the Jest mock):

// Given
const listBuckets = jest.fn().mockResolvedValue({ Buckets: [] });
awsMock.mock('S3', 'listBuckets', listBuckets);

// When
await new aws.S3().listBuckets({}).promise();

// Then
expect(listBuckets).toHaveBeenCalledWith({});

[mjdickinson]

No, it behaves the same. That's actually closer to how our unit tests are written. We set default mocks on AWS service methods in shared test setup and then override with specific implementation in individual tests.

…

On Fri, Jan 7, 2022 at 1:40 AM Thomas Anciaux ***@***.***> wrote: Thanks for the details @mjdickinson <https://github.com/mjdickinson>, it was my understanding the behaviour you are seeking was still supported. I'll reproduce this example and see what's going wrong in 5.5.1. For the record, the whole reason I initially suggested the changes for 5.5.0 was to make it easier to test using Jest. I didn't want to manually have to wrap my Jest Mocks before passing them to the library. In my projects we're exclusively using promise based method calls, which is why I missed the potential issues with callbacks and hence the reason why we fixed it in 5.5.1. Just out of curiosity, does the following approach work (notice the change in how I defined the Jest mock): // Givenconst listBuckets = jest.fn().mockResolvedValue({ Buckets: [] });awsMock.mock('S3', 'listBuckets', listBuckets); // Whenawait new aws.S3().listBuckets({}).promise(); // Thenexpect(listBuckets).toHaveBeenCalledWith({}); — Reply to this email directly, view it on GitHub <#256 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAOCF4YTZV252GCFPOMDUZLUU2YJTANCNFSM5JJQSDPQ> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>. You are receiving this because you were mentioned.Message ID: ***@***.***>

[aardvarkk]

I'm not familiar with this issue, but it seems related to a test failure we're getting after upgrading from 5.5.0.

After upgrading to 5.5.1 , we have a Jest test that fails saying it's receiving an extra argument, so we had to add expect.any(Function) on the end of toHaveBeenCalledWith.

[thomaux]

Thanks for the input @aardvarkk , are your tests defined in a similar way to those of @mjdickinson?

[aardvarkk]

@thomaux Our test looks like this:

  it("should send a confirmation link to the supplied email", async () => {
    const sesMockFn = jest.fn(() => Promise.resolve());

    AWSMock.setSDKInstance(AWS);
    AWSMock.mock("SES", "sendEmail", sesMockFn);

    const email = "email@address.com";

    // This function calls out to SES to send the email
    const response = await signUp({
      body: {
        email,
      },
    });

    expect(sesMockFn).toHaveBeenCalledWith(
      expect.objectContaining({
        Destination: expect.objectContaining({
          ToAddresses: expect.arrayContaining([email]),
        }),
      }),
      expect.any(Function) // Required for aws-sdk-mock v5.5.1
    );

    expect(response.statusCode).toBe(200);
  });

Previously, we didn't need that second argument (expect.any(Function)) to be passed to toHaveBeenCalledWith, but the test failed without it after upgrading to v5.5.1

[loopingz]

I did not had time to investigate more but the 5.5.x update are still breaking working tests: loopingz/webda.io#148