dwyl / aws-sdk-mock

:rainbow: AWSomocks for Javascript/Node.js aws-sdk tested, documented & maintained. Contributions welcome!
Apache License 2.0
1.1k stars 109 forks source link

Not working with individually required client services? #154

Open rkulla opened 5 years ago

rkulla commented 5 years ago

When my real code requires just an individual service, like just SecretsManager, aws-mock seems to call out to the real SecretsManager's getSecretValue method. Shortened code for the sake of example:

// getAWSSecret.js
const SecretsManager = require('aws-sdk/clients/secretsmanager');
// const aws = require('aws-sdk');

const getAWSSecret = (secretId) => {
    // const sm = new aws.SecretsManager({apiVersion: SECRET_API_VERSION, region: REGION});
    const sm = new SecretsManager({apiVersion: SECRET_API_VERSION, region: REGION});

    return new Promise((resolve, reject) => {
        sm.getSecretValue({ SecretId: secretId }, (err, data) => {
            ...
    });
    ...
}

// secretsmanager.jest.test.js 
const getAWSSecret = require('../getAWSSecret');
const secretResponse = require('./../../fixtures/getSecretValue');
const aws = require('aws-sdk-mock');
describe('getAWSSecret Test', () => {

beforeEach(() => {
  aws.mock('SecretsManager', 'getSecretValue', secretResponse);
});

afterEach(() => {
   aws.restore('SecretsManager');
 });

 test('getAWSSecret valid mock test ',  () => {
  return getAWSSecret("secretId").then((secretResponse) => {
    expect(secretResponse).toBeDefined();
    ...
  });
});
...

So the commented out stuff above works fine, but I don't want to have to require the entire aws-sdk into memory in my real code (it's used by AWS Lambda functions, which need the memory) just to enable mocking of one service.

Is there a way to do this with aws-sdk-mock?

nelsonic commented 5 years ago

@rkulla good question! thank you for opening this issue to share your use case. 👍 We have not had this requirement before and I'm not aware of how much RAM will be saved on AWS Lambda from requiring an individual service like this, could you confirm that it does indeed work and reduces RAM usage? (that would be awesome!) We would definitely add tests/code to support this functionality if it shaved off RAM usage on Lambda. thanks!

rkulla commented 5 years ago

I'm not sure of exact size but I got that from https://docs.aws.amazon.com/lambda/latest/dg/best-practices.html which says Minimize your deployment package size to its runtime necessities. This will reduce the amount of time that it takes for your deployment package to be downloaded and unpacked ahead of invocation. For functions authored in Java or .NET Core, avoid uploading the entire AWS SDK library as part of your deployment package. Instead, selectively depend on the modules which pick up components of the SDK you need (e.g. DynamoDB, Amazon S3 SDK modules and Lambda core libraries).

I have also read that's suggested in random tutorials for the JS aws-sdk i've read and it's also mentioned here https://github.com/aws/aws-sdk-js

rkulla commented 5 years ago

Also, the aws-sdk for javascript is already pre-installed with aws-lambda so this is more a question of loading the entire sdk into memory when doing require statements (So i'm not really concerned with deployment package size). Here's another good link https://docs.aws.amazon.com/sdk-for-javascript/v2/developer-guide/creating-and-calling-service-objects.html#requiring-individual-services

Clete2 commented 5 years ago

+1. I struggled with this same issue (literally) all day without knowing it. Looking through the aws-sdk-mock code, it seems that it's traversing the code as if you are importing them directly from the top-level SDK.

I did find one way that still looks clean and works with aws-sdk-mock.

Import like this: import { DynamoDB } from 'aws-sdk'

In my webpack.config.js I added externals: /^aws-sdk/, to completely exclude the aws-sdk from my Lambdas. You can do this because the Lambda runtime already provides aws-sdk. After doing that, it doesn't matter how you import because the SDK never gets bundled.

carlbergman commented 5 years ago

I would like to briefly summarize the problems and suggested solution behind this issue.

Problem It is not possible to stub clients that have been required directly, like this:

var SNS = require('aws-sdk/clients/sns');

From what I understand, Sinon can not stub it because it is not wrapped in another module.

Workaround

Require the entire SDK instead, like this:

var AWS = require('aws-sdk');

Caveats

  1. Requiring the entire SDK is not recommended by AWS, as mentioned by @rkulla. It has performance implications, both when being run locally and in the cloud.
  2. Excluding the SDK from the bundle, like @Clete2 suggests, means that you have to rely on the SDK version that is included in the Lambda execution environment. Currently it is 2.290.0 while the latest on NPM is 2.437.0. Features and improvements will be missing.

Questions

harleyguru commented 5 years ago

Really, this issue should be fixed.. Importing individual service module from AWS SDK is the basic and best practice. For just testing purpose, we can't make our code ineffective.

Please fix this issue..

JimBeem commented 5 years ago

In the mean time, you can mock these services with Sinon alone if that helps you test until this package comes up to speed.

In my production code:

import { AWSError, CognitoIdentityServiceProvider } from 'aws-sdk';

// proceed to new up and use CISP object as necessary

In my test code

import * as AWS from 'aws-sdk';

// stubBox is just a Sinon sandbox

adminAddUserToGroupStub = stubBox.stub();
adminCreateUserStub = stubBox.stub();
listUsersStub = stubBox.stub();
adminListGroupsForUserStub = stubBox.stub();
adminUpdateUserAttributesStub = stubBox.stub();
adminDisableUserStub = stubBox.stub();   
adminEnableUserStub = stubBox.stub();
// Actual CISP stub
stubBox.stub(AWS, 'CognitoIdentityServiceProvider').returns({
    adminCreateUser: adminCreateUserStub,
    adminAddUserToGroup: adminAddUserToGroupStub,
    listUsers: listUsersStub,
    adminListGroupsForUser: adminListGroupsForUserStub,
    adminUpdateUserAttributes: adminUpdateUserAttributesStub,
    adminDisableUser: adminDisableUserStub,
    adminEnableUser: adminEnableUserStub,
});

Is it convenient? no. But it works for now.