DataDog / serverless-plugin-datadog

Serverless plugin to automagically instrument your Lambda functions with Datadog
Apache License 2.0
96 stars 49 forks source link

chore: Add integration test for getRecommendedMonitors() #549

Closed lym953 closed 3 weeks ago

lym953 commented 3 weeks ago

What does this PR do?

  1. Add an integration test for getRecommendedMonitors(), which calls /api/v2/monitor/recommended API. This test ensures we can fetch the recommended monitors properly.
  2. Create a seperate "npm run" command to run this test: npm run test:integration. Running it in a separate command because: (1) this test access the Internet, which could be slow (2) this test requires DD_API_KEY and DD_APP_KEY, involving additional setup, and the developer may not want to do it.
  3. Create separate jest config files for the two commands: one for running tests src/*.spec.ts, one for integration_tests/*.spec.ts
  4. Make GitHub Action run the added test

Motivation

In https://github.com/DataDog/serverless-plugin-datadog/issues/545 and incident 31902, getRecommendedMonitors() failed to fetch monitors due to a change in the response. This issue was reported by customers 1+ months after being introduced. If we had this test and ran it regularly with notification set up, we could have caught it earlier.

Testing Guidelines

npm run test:integration passes.

It will fail if I make getExistingMonitors() return nothing.

export async function getRecommendedMonitors(
  _site: string,
  _monitorsApiKey: string,
  _monitorsAppKey: string,
): Promise<{
  [key: string]: ServerlessMonitor;
}> {
  return {};
}
image

Additional Notes

Next steps:

  1. Make this GitHub action run periodically
  2. If the test fails, send a notification to team's ops channel

Types of changes

Check all that apply

lym953 commented 3 weeks ago

/merge

dd-devflow[bot] commented 3 weeks ago

:steam_locomotive: MergeQueue: waiting for PR to be ready

This merge request is not mergeable yet, because of pending checks/missing approvals. It will be added to the queue as soon as checks pass and/or get approvals. Note: if you pushed new commits since the last approval, you may need additional approval. You can remove it from the waiting list with /remove command.

Use /merge -c to cancel this operation!

lym953 commented 3 weeks ago

Adding back the deleted jest config in package.json.

  "jest": {
    "verbose": true,
    "moduleFileExtensions": [
      "ts",
      "tsx",
      "js"
    ],
    "transform": {
      ".(ts|tsx)": "ts-jest"
    },
    "collectCoverage": true,
    "coverageReporters": [
      "lcovonly",
      "text-summary"
    ],
    "testRegex": "(src\\/).*(\\.spec\\.ts)$",
    "testPathIgnorePatterns": [
      "\\.snap$",
      "<rootDir>/node_modules/"
    ],
    "testEnvironment": "<rootDir>/testEnvironment.js",
    "clearMocks": true,
    "collectCoverageFrom": [
      "src/**/*.ts"
    ]
  },

I think they are still using it:

    "test:watch": "jest --watch",
    "coverage": "jest --coverage",
dd-devflow[bot] commented 3 weeks ago

:steam_locomotive: MergeQueue: pull request added to the queue

The median merge time in main is 2m.

Use /merge -c to cancel this operation!