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

rest() and resetBehavior() stops onAnyCommand() working (DynamoDB) #67

Closed
1 task done
si-c613 opened this issue Jan 6, 2022 · 2 comments · Fixed by #76
Closed
1 task done

rest() and resetBehavior() stops onAnyCommand() working (DynamoDB) #67

si-c613 opened this issue Jan 6, 2022 · 2 comments · Fixed by #76
Labels
bug Something isn't working

Comments

@si-c613
Copy link

si-c613 commented Jan 6, 2022

Checklist

  • I have read Caveats documentation and didn't find a solution for this problem there.

Bug description

When using the reset() function between tests and using onAnyCommand() to set a default response there is no error however the default response does not seem to work.

As part of debugging the issue it was noticed in the comments that reset() basically performs resetHistory() and resetBehavior(). We tried setting these individually and (as expected) were able to pin it down to just the resetBehavior() function.

Rightly or wrongly to avoid asserting the arguments of the function call I was using the onAnyCommand().rejects() to reject any calls by default and then using more specific on() to catch the calls I was expecting.
This all works but when it came to mutation testing and deliberately trying to force tests to fail if the code isn't as expected the tests were passing when in specific orders.
The first test would use the onAnyCommand() response as expected, between tests (using beforeEach()) we use reset() to remove any behaviours and history and then do onAnyCommand().rejects() again, however in subsequent tests we do not see this being used and instead the mock resolves the promise.

In the below we expect both tests to pass which they do, however if we change the service name on L30 to something else, NOT-my-service for example, the test passes when it should fail. Run the same test in isolation and it fails as expected.
(for clarity this is a slightly simplified version to help convey the issue, we are making mutations in the .ts not the .test.ts which is why this may seem an odd way of doing things!)

// bug-report.test.ts
import {mockClient} from 'aws-sdk-client-mock'
import {DeleteItemCommand, DynamoDBClient} from '@aws-sdk/client-dynamodb'
import {removeLock} from './bug-report'

const ddbMock = mockClient(DynamoDBClient)
const WrongParamsErr = new Error('Incorrect call params (without this mock will resolve any call!)')

describe('Remove lock', () => {
  beforeEach(() => {
    ddbMock.reset()
    ddbMock.onAnyCommand().rejects(WrongParamsErr)
  })

  test('throws decorated error if delete command fails', async () => {
    expect.assertions(1)
    const err = new Error('mock failed error')
    ddbMock.on(DeleteItemCommand).rejects(err)
    await expect(removeLock('my-service')).rejects.toThrow(`Failed removing lock from DDB (my-service): ${err}`)
  })

  test('makes expected calls to DDB SDK and logs', async () => {
    expect.assertions(1)
    const args = {
      Key: {lock: {S: 'NOT-my-service'}},
      TableName: 'my-table'
    }
    ddbMock
      .on(DeleteItemCommand, args)
      .resolves({})
    await expect(removeLock('my-service')).resolves.not.toThrow()
  })
})
// bug-resport.ts
import {DeleteItemCommand, DynamoDBClient} from '@aws-sdk/client-dynamodb'

const DDB = new DynamoDBClient({region: 'eu-central-1'})

export async function removeLock(serviceName: string): Promise<void> {
  try {
    const command = new DeleteItemCommand({
      Key: {lock: {S: serviceName}},
      TableName: 'my-table'
    })
    await DDB.send(command)
  } catch (err) {
    throw new Error(`Failed removing lock from DDB (${serviceName}): ${err}`)
  }
}

Final few observations, if we switch the order of the tests then the mutation fails as expected.
Also strangely if we use the below as our describe block, both tests pass as expected, if we change the assertion to removeLock('NOT-my-service'), the test fails, as expected. Yet changing the mock to Key: {lock: {S: 'NOT-my-service'}}, leaving the assertion as the original removeLock('my-service') the test passes when the same fail is expected.

describe('Remove lock', () => {
  beforeEach(() => {
    ddbMock.reset()
    ddbMock.onAnyCommand().rejects(WrongParamsErr)
  })

  test('makes expected calls to DDB SDK and logs', async () => {
    expect.assertions(1)
    const args = {
      Key: {lock: {S: 'my-service'}},
      TableName: 'my-table'
    }
    ddbMock
      .on(DeleteItemCommand, args)
      .resolves({})
    await expect(removeLock('my-service')).resolves.not.toThrow()
  })

  test('makes expected calls to DDB SDK and logs', async () => {
    expect.assertions(1)
    const args = {
      Key: {lock: {S: 'my-service'}},
      TableName: 'my-table'
    }
    ddbMock
      .on(DeleteItemCommand, args)
      .resolves({})
    await expect(removeLock('my-service')).resolves.not.toThrow()
  })
})

Environment

  • Node version: v12.22.7
  • Testing lib and version: Jest 27.4.5
  • Typescript version: 4.5.4
  • AWS SDK v3 Client mock version: 0.5.6
  • AWS JS SDK libs and versions:
    • @aws-sdk/client-dynamodb v3.44.0
@si-c613 si-c613 added the bug Something isn't working label Jan 6, 2022
@m-radzikowski
Copy link
Owner

Hey,
thank you for the detailed report and sorry for the delay.

I'm investigating it right now. I was able to simplify this to:

test('onAnyCommand works again after reset', async () => {
    ddbMock.onAnyCommand().rejects('any command mock error')

    ddbMock.on(DeleteItemCommand).rejects(new Error('mock failed error'))

    const sendDeleteItem1 = ddb.send(new DeleteItemCommand({
        TableName: 'my-table',
        Key: {pk: {S: 'qq'}}
    }))

    await expect(sendDeleteItem1).rejects.toThrow('mock failed error');

    ddbMock.reset()
    ddbMock.onAnyCommand().rejects('any command mock error')

    const sendDeleteItem2 = ddb.send(new DeleteItemCommand({
        TableName: 'my-table',
        Key: {pk: {S: 'qq'}}
    }))

    await expect(sendDeleteItem2).rejects.toThrow('any command mock error');
});

Right now the last assertion throws:

Matcher error: received value must be a promise or a function returning a promise
Received has value: undefined

So after the reset() the onAnyCommand() did not set the mocks. However, replacing it with on(DeleteItemCommand) mock works fine, so it's a problem with onAnyCommand() only.

Interestingly, if we remove the first onAnyCommand() OR on(DeleteItemCommand) then it works 🤷

I will update you when I have some solutions!

@m-radzikowski
Copy link
Owner

Released in 0.6.0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants