From 13099195dbe8e6df94e07f18ef8f4fdbe15080e8 Mon Sep 17 00:00:00 2001 From: Seth Silesky <5115498+silesky@users.noreply.github.com> Date: Thu, 13 Jul 2023 11:13:41 -0500 Subject: [PATCH] Add eslint-plugin-jest and jest rules; fix tests that violate eslint rules (#883) --- .eslintrc.js | 9 ++ .husky/pre-push | 2 +- package.json | 1 + .../analytics-pre-init.integration.test.ts | 15 +-- .../src/browser/__tests__/integration.test.ts | 6 +- .../src/core/buffer/__tests__/index.test.ts | 3 +- .../src/core/events/__tests__/index.test.ts | 1 + .../lib/client-hints/__tests__/index.test.ts | 30 +---- .../__tests__/integration.test.ts | 74 ------------ .../plugins/segmentio/__tests__/index.test.ts | 4 +- .../segmentio/__tests__/normalize.test.ts | 4 - .../src/test-helpers/fixtures/client-hints.ts | 28 +++++ .../src/queue/__tests__/event-queue.test.ts | 25 ---- .../src/__tests__/http-integration.test.ts | 2 +- .../node/src/__tests__/integration.test.ts | 6 +- yarn.lock | 111 ++++++++++++++++++ 16 files changed, 173 insertions(+), 148 deletions(-) delete mode 100644 packages/browser/src/plugins/remote-loader/__tests__/integration.test.ts create mode 100644 packages/browser/src/test-helpers/fixtures/client-hints.ts diff --git a/.eslintrc.js b/.eslintrc.js index 3345f1a90..b52fb4eb5 100644 --- a/.eslintrc.js +++ b/.eslintrc.js @@ -31,6 +31,8 @@ module.exports = { 'plugin:@typescript-eslint/recommended', // Handle prettier rules through eslint https://github.com/prettier/eslint-plugin-prettier/blob/master/eslint-plugin-prettier.js#L65 'plugin:prettier/recommended', + // add special jest rules https://github.com/jest-community/eslint-plugin-jest#rules + 'plugin:jest/recommended', ], rules: { '@typescript-eslint/explicit-function-return-type': 'off', @@ -50,6 +52,13 @@ module.exports = { '@typescript-eslint/require-await': 'error', '@typescript-eslint/no-empty-function': 'off', '@typescript-eslint/ban-types': 'off', // TODO: turn on + /** jest */ + 'jest/valid-title': 'off', // allow functions to be used as describe titles + 'jest/no-conditional-expect': 'off', // best practice, but TODO + 'jest/no-alias-methods': 'off', // best practice, but TODO + 'jest/expect-expect': 'off', // sometimes we compose assertion functions + 'jest/no-disabled-tests': process.env.CI ? 'error' : 'off', // fail CI if tests are disabled, but do not interfere with local development + 'jest/no-focused-tests': process.env.CI ? 'error' : 'off', }, overrides: [ { diff --git a/.husky/pre-push b/.husky/pre-push index f077c9172..9f948b6f2 100755 --- a/.husky/pre-push +++ b/.husky/pre-push @@ -1,4 +1,4 @@ #!/bin/sh . "$(dirname "$0")/_/husky.sh" -yarn test +CI=true yarn test --colors --silent diff --git a/package.json b/package.json index 6554dafee..6853ba8a5 100644 --- a/package.json +++ b/package.json @@ -48,6 +48,7 @@ "eslint": "^8.14.0", "eslint-config-prettier": "^8.5.0", "eslint-plugin-import": "^2.27.5", + "eslint-plugin-jest": "^27.2.2", "eslint-plugin-prettier": "^4.0.0", "express": "^4.18.2", "get-monorepo-packages": "^1.2.0", diff --git a/packages/browser/src/browser/__tests__/analytics-pre-init.integration.test.ts b/packages/browser/src/browser/__tests__/analytics-pre-init.integration.test.ts index 612d82c61..7fd14c276 100644 --- a/packages/browser/src/browser/__tests__/analytics-pre-init.integration.test.ts +++ b/packages/browser/src/browser/__tests__/analytics-pre-init.integration.test.ts @@ -141,14 +141,14 @@ describe('Pre-initialization', () => { describe('Promise API', () => { describe('.then', () => { - test('.then should be called on success', (done) => { + test('.then should be called on success', () => { const ajsBrowser = AnalyticsBrowser.load({ writeKey: 'abc' }) const newPromise = ajsBrowser.then(([analytics, context]) => { expect(analytics).toBeInstanceOf(Analytics) expect(context).toBeInstanceOf(Context) - done() }) expect(newPromise).toBeInstanceOf(Promise) + return newPromise }) it('.then should pass to the next .then', async () => { @@ -160,15 +160,12 @@ describe('Pre-initialization', () => { }) describe('.catch', () => { - it('should be capable of handling errors if using promise syntax', () => { + it('should be capable of handling errors if using promise syntax', async () => { browserLoadSpy.mockImplementationOnce((): any => Promise.reject(errMsg)) - const ajsBrowser = AnalyticsBrowser.load({ writeKey: 'abc' }) - const newPromise = ajsBrowser.catch((reason) => { - expect(reason).toBe(errMsg) - }) - expect(newPromise).toBeInstanceOf(Promise) - expect.assertions(2) + await expect(() => + AnalyticsBrowser.load({ writeKey: 'abc' }) + ).rejects.toEqual(errMsg) }) }) diff --git a/packages/browser/src/browser/__tests__/integration.test.ts b/packages/browser/src/browser/__tests__/integration.test.ts index daff261ab..e9b751902 100644 --- a/packages/browser/src/browser/__tests__/integration.test.ts +++ b/packages/browser/src/browser/__tests__/integration.test.ts @@ -18,11 +18,11 @@ import { getCDN, setGlobalCDNUrl } from '../../lib/parse-cdn' import { clearAjsBrowserStorage } from '../../test-helpers/browser-storage' import { parseFetchCall } from '../../test-helpers/fetch-parse' import { ActionDestination } from '../../plugins/remote-loader' +import { UADataValues } from '../../lib/client-hints/interfaces' import { highEntropyTestData, lowEntropyTestData, -} from '../../lib/client-hints/__tests__/index.test' -import { UADataValues } from '../../lib/client-hints/interfaces' +} from '../../test-helpers/fixtures/client-hints' let fetchCalls: ReturnType[] = [] @@ -1083,7 +1083,7 @@ describe('Options', () => { expect(integrationEvent.timestamp()).toBeInstanceOf(Date) }) - it('converts iso strings to dates be default', async () => { + it('does not convert iso strings to dates be default if disableAutoISOConversion is false', async () => { const initOptions: InitOptions = { disableAutoISOConversion: false } const [analytics] = await AnalyticsBrowser.load( { diff --git a/packages/browser/src/core/buffer/__tests__/index.test.ts b/packages/browser/src/core/buffer/__tests__/index.test.ts index b2c3e1399..071a4b6c1 100644 --- a/packages/browser/src/core/buffer/__tests__/index.test.ts +++ b/packages/browser/src/core/buffer/__tests__/index.test.ts @@ -204,7 +204,7 @@ describe('AnalyticsBuffered', () => { test('Will ignore the .then if there is a catch block', () => { const thenCb = jest.fn() - new AnalyticsBuffered(() => { + const p = new AnalyticsBuffered(() => { return Promise.reject('nope') as any }) .then(() => { @@ -215,6 +215,7 @@ describe('AnalyticsBuffered', () => { }) expect(thenCb).not.toBeCalled() expect.assertions(2) + return p }) }) }) diff --git a/packages/browser/src/core/events/__tests__/index.test.ts b/packages/browser/src/core/events/__tests__/index.test.ts index 86e6ecd0d..1b939011f 100644 --- a/packages/browser/src/core/events/__tests__/index.test.ts +++ b/packages/browser/src/core/events/__tests__/index.test.ts @@ -331,6 +331,7 @@ describe('Event Factory', () => { }) }) + // eslint-disable-next-line jest/no-disabled-tests describe.skip('anonymousId', () => { // TODO: the code should be fixed so that these tests can pass -- this eventFactory does not seem to handle these edge cases well. // When an event is dispatched, there are four places anonymousId can live: event.anonymousId, event.options.anonymousId, event.context.anonymousId, and the user object / localStorage. diff --git a/packages/browser/src/lib/client-hints/__tests__/index.test.ts b/packages/browser/src/lib/client-hints/__tests__/index.test.ts index 728dbe0c5..de0f6232e 100644 --- a/packages/browser/src/lib/client-hints/__tests__/index.test.ts +++ b/packages/browser/src/lib/client-hints/__tests__/index.test.ts @@ -1,29 +1,9 @@ import { clientHints } from '..' -import { UADataValues, UALowEntropyJSON } from '../interfaces' - -export const lowEntropyTestData: UALowEntropyJSON = { - brands: [ - { - brand: 'Google Chrome', - version: '113', - }, - { - brand: 'Chromium', - version: '113', - }, - { - brand: 'Not-A.Brand', - version: '24', - }, - ], - mobile: false, - platform: 'macOS', -} - -export const highEntropyTestData: UADataValues = { - architecture: 'x86', - bitness: '64', -} +import { + highEntropyTestData, + lowEntropyTestData, +} from '../../../test-helpers/fixtures/client-hints' +import { UADataValues } from '../interfaces' describe('Client Hints API', () => { beforeEach(() => { diff --git a/packages/browser/src/plugins/remote-loader/__tests__/integration.test.ts b/packages/browser/src/plugins/remote-loader/__tests__/integration.test.ts deleted file mode 100644 index 49b6ec1a6..000000000 --- a/packages/browser/src/plugins/remote-loader/__tests__/integration.test.ts +++ /dev/null @@ -1,74 +0,0 @@ -import { JSDOM } from 'jsdom' -import { AnalyticsBrowser, LegacySettings } from '../../../browser' - -jest.mock('unfetch', () => { - return jest.fn() -}) - -import unfetch from 'unfetch' - -describe.skip('Remote Plugin Integration', () => { - const window = global.window as any - - beforeEach(async () => { - jest.restoreAllMocks() - jest.resetAllMocks() - - const html = ` - - - - - - - - `.trim() - - const jsd = new JSDOM(html, { - runScripts: 'dangerously', - resources: 'usable', - url: 'https://localhost', - }) - - const windowSpy = jest.spyOn(global, 'window', 'get') - windowSpy.mockImplementation( - () => jsd.window as unknown as Window & typeof globalThis - ) - - const cdnResponse: LegacySettings = { - integrations: {}, - remotePlugins: [ - // This may be a bit flaky - // we should mock this file in case it becomes a problem - // but I'd like to have a full integration test if possible - { - name: 'amplitude', - creationName: 'amplitude', - url: 'https://ajs-next-integrations.s3-us-west-2.amazonaws.com/fab-5/amplitude-plugins.js', - libraryName: 'amplitude-pluginsDestination', - settings: { - subscriptions: `[{"partnerAction":"sessionId","name":"SessionId","enabled":true,"subscribe":"type = \\"track\\"", "mapping":{}}]`, - }, - }, - ], - } - - // @ts-ignore mocking fetch is *hard* - jest.mocked(unfetch).mockImplementation( - (): Promise => - // @ts-expect-error - Promise.resolve({ - json: () => Promise.resolve(cdnResponse), - }) - ) - }) - - it('loads remote plugins', async () => { - await AnalyticsBrowser.load({ - writeKey: 'test-write-key', - }) - - // loaded remote plugin - expect(window['amplitude-pluginsDestination']).not.toBeUndefined() - }) -}) diff --git a/packages/browser/src/plugins/segmentio/__tests__/index.test.ts b/packages/browser/src/plugins/segmentio/__tests__/index.test.ts index d95ababae..5f3394d45 100644 --- a/packages/browser/src/plugins/segmentio/__tests__/index.test.ts +++ b/packages/browser/src/plugins/segmentio/__tests__/index.test.ts @@ -5,11 +5,11 @@ import { Analytics } from '../../../core/analytics' import { Plugin } from '../../../core/plugin' import { pageEnrichment } from '../../page-enrichment' import cookie from 'js-cookie' +import { UADataValues } from '../../../lib/client-hints/interfaces' import { highEntropyTestData, lowEntropyTestData, -} from '../../../lib/client-hints/__tests__/index.test' -import { UADataValues } from '../../../lib/client-hints/interfaces' +} from '../../../test-helpers/fixtures/client-hints' jest.mock('unfetch', () => { return jest.fn() diff --git a/packages/browser/src/plugins/segmentio/__tests__/normalize.test.ts b/packages/browser/src/plugins/segmentio/__tests__/normalize.test.ts index 9f4835bdd..a91a43e5f 100644 --- a/packages/browser/src/plugins/segmentio/__tests__/normalize.test.ts +++ b/packages/browser/src/plugins/segmentio/__tests__/normalize.test.ts @@ -371,10 +371,6 @@ describe('before loading', () => { assert(!object.context.amp) }) - describe('failed initializations', () => { - it.skip('should add failedInitializations as part of _metadata object if this.analytics.failedInitilizations is not empty', () => {}) - }) - describe('unbundling', () => { it('should add a list of bundled integrations', () => { normalize(analytics, object, options, { diff --git a/packages/browser/src/test-helpers/fixtures/client-hints.ts b/packages/browser/src/test-helpers/fixtures/client-hints.ts new file mode 100644 index 000000000..d0240b4bc --- /dev/null +++ b/packages/browser/src/test-helpers/fixtures/client-hints.ts @@ -0,0 +1,28 @@ +import { + UADataValues, + UALowEntropyJSON, +} from '../../lib/client-hints/interfaces' + +export const lowEntropyTestData: UALowEntropyJSON = { + brands: [ + { + brand: 'Google Chrome', + version: '113', + }, + { + brand: 'Chromium', + version: '113', + }, + { + brand: 'Not-A.Brand', + version: '24', + }, + ], + mobile: false, + platform: 'macOS', +} + +export const highEntropyTestData: UADataValues = { + architecture: 'x86', + bitness: '64', +} diff --git a/packages/core/src/queue/__tests__/event-queue.test.ts b/packages/core/src/queue/__tests__/event-queue.test.ts index 34a887b9f..98c984df5 100644 --- a/packages/core/src/queue/__tests__/event-queue.test.ts +++ b/packages/core/src/queue/__tests__/event-queue.test.ts @@ -648,31 +648,6 @@ describe('dispatchSingle', () => { expect(eq.queue.length).toBe(0) }) - it.skip('records delivery metrics', async () => { - // Skip because we don't support metrics atm - const eq = new TestEventQueue() - const ctx = await eq.dispatchSingle( - new TestCtx({ - type: 'track', - }) - ) - - expect(ctx.logs().map((l) => l.message)).toMatchInlineSnapshot(` - Array [ - "Dispatching", - "Delivered", - ] - `) - - expect(ctx.stats.metrics.map((m) => m.metric)).toMatchInlineSnapshot(` - Array [ - "message_dispatched", - "message_delivered", - "delivered", - ] - `) - }) - test('retries retriable cancelations', async () => { // make sure all backoffs return immediatelly jest.spyOn(timer, 'backoff').mockImplementationOnce(() => 100) diff --git a/packages/node/src/__tests__/http-integration.test.ts b/packages/node/src/__tests__/http-integration.test.ts index 1599ba705..e23e360dd 100644 --- a/packages/node/src/__tests__/http-integration.test.ts +++ b/packages/node/src/__tests__/http-integration.test.ts @@ -87,7 +87,7 @@ describe('Method Smoke Tests', () => { ], } `) - expect(scope.isDone()) + expect(scope.isDone()).toBeTruthy() }) }) diff --git a/packages/node/src/__tests__/integration.test.ts b/packages/node/src/__tests__/integration.test.ts index 0bf909bad..9c5ef61d0 100644 --- a/packages/node/src/__tests__/integration.test.ts +++ b/packages/node/src/__tests__/integration.test.ts @@ -338,7 +338,7 @@ describe('ready', () => { expect(analytics['_queue'].plugins.length).toBeGreaterThan(0) }) - it.skip('should not reject if a plugin fails registration during initialization?', async () => { - // TODO: we should test the unhappy path - }) + it.todo( + 'should not reject if a plugin fails registration during initialization?' + ) }) diff --git a/yarn.lock b/yarn.lock index 7b410b0de..fa031fa72 100644 --- a/yarn.lock +++ b/yarn.lock @@ -904,6 +904,17 @@ __metadata: languageName: node linkType: hard +"@eslint-community/eslint-utils@npm:^4.2.0": + version: 4.4.0 + resolution: "@eslint-community/eslint-utils@npm:4.4.0" + dependencies: + eslint-visitor-keys: ^3.3.0 + peerDependencies: + eslint: ^6.0.0 || ^7.0.0 || >=8.0.0 + checksum: cdfe3ae42b4f572cbfb46d20edafe6f36fc5fb52bf2d90875c58aefe226892b9677fef60820e2832caf864a326fe4fc225714c46e8389ccca04d5f9288aabd22 + languageName: node + linkType: hard + "@eslint/eslintrc@npm:^1.2.2": version: 1.2.2 resolution: "@eslint/eslintrc@npm:1.2.2" @@ -3842,6 +3853,13 @@ __metadata: languageName: node linkType: hard +"@types/semver@npm:^7.3.12": + version: 7.5.0 + resolution: "@types/semver@npm:7.5.0" + checksum: 0a64b9b9c7424d9a467658b18dd70d1d781c2d6f033096a6e05762d20ebbad23c1b69b0083b0484722aabf35640b78ccc3de26368bcae1129c87e9df028a22e2 + languageName: node + linkType: hard + "@types/send@npm:*": version: 0.17.1 resolution: "@types/send@npm:0.17.1" @@ -4025,6 +4043,16 @@ __metadata: languageName: node linkType: hard +"@typescript-eslint/scope-manager@npm:5.62.0": + version: 5.62.0 + resolution: "@typescript-eslint/scope-manager@npm:5.62.0" + dependencies: + "@typescript-eslint/types": 5.62.0 + "@typescript-eslint/visitor-keys": 5.62.0 + checksum: 6062d6b797fe1ce4d275bb0d17204c827494af59b5eaf09d8a78cdd39dadddb31074dded4297aaf5d0f839016d601032857698b0e4516c86a41207de606e9573 + languageName: node + linkType: hard + "@typescript-eslint/type-utils@npm:5.21.0": version: 5.21.0 resolution: "@typescript-eslint/type-utils@npm:5.21.0" @@ -4048,6 +4076,13 @@ __metadata: languageName: node linkType: hard +"@typescript-eslint/types@npm:5.62.0": + version: 5.62.0 + resolution: "@typescript-eslint/types@npm:5.62.0" + checksum: 48c87117383d1864766486f24de34086155532b070f6264e09d0e6139449270f8a9559cfef3c56d16e3bcfb52d83d42105d61b36743626399c7c2b5e0ac3b670 + languageName: node + linkType: hard + "@typescript-eslint/typescript-estree@npm:5.21.0": version: 5.21.0 resolution: "@typescript-eslint/typescript-estree@npm:5.21.0" @@ -4066,6 +4101,24 @@ __metadata: languageName: node linkType: hard +"@typescript-eslint/typescript-estree@npm:5.62.0": + version: 5.62.0 + resolution: "@typescript-eslint/typescript-estree@npm:5.62.0" + dependencies: + "@typescript-eslint/types": 5.62.0 + "@typescript-eslint/visitor-keys": 5.62.0 + debug: ^4.3.4 + globby: ^11.1.0 + is-glob: ^4.0.3 + semver: ^7.3.7 + tsutils: ^3.21.0 + peerDependenciesMeta: + typescript: + optional: true + checksum: 3624520abb5807ed8f57b1197e61c7b1ed770c56dfcaca66372d584ff50175225798bccb701f7ef129d62c5989070e1ee3a0aa2d84e56d9524dcf011a2bb1a52 + languageName: node + linkType: hard + "@typescript-eslint/utils@npm:5.21.0": version: 5.21.0 resolution: "@typescript-eslint/utils@npm:5.21.0" @@ -4082,6 +4135,24 @@ __metadata: languageName: node linkType: hard +"@typescript-eslint/utils@npm:^5.10.0": + version: 5.62.0 + resolution: "@typescript-eslint/utils@npm:5.62.0" + dependencies: + "@eslint-community/eslint-utils": ^4.2.0 + "@types/json-schema": ^7.0.9 + "@types/semver": ^7.3.12 + "@typescript-eslint/scope-manager": 5.62.0 + "@typescript-eslint/types": 5.62.0 + "@typescript-eslint/typescript-estree": 5.62.0 + eslint-scope: ^5.1.1 + semver: ^7.3.7 + peerDependencies: + eslint: ^6.0.0 || ^7.0.0 || ^8.0.0 + checksum: ee9398c8c5db6d1da09463ca7bf36ed134361e20131ea354b2da16a5fdb6df9ba70c62a388d19f6eebb421af1786dbbd79ba95ddd6ab287324fc171c3e28d931 + languageName: node + linkType: hard + "@typescript-eslint/visitor-keys@npm:5.21.0": version: 5.21.0 resolution: "@typescript-eslint/visitor-keys@npm:5.21.0" @@ -4092,6 +4163,16 @@ __metadata: languageName: node linkType: hard +"@typescript-eslint/visitor-keys@npm:5.62.0": + version: 5.62.0 + resolution: "@typescript-eslint/visitor-keys@npm:5.62.0" + dependencies: + "@typescript-eslint/types": 5.62.0 + eslint-visitor-keys: ^3.3.0 + checksum: 976b05d103fe8335bef5c93ad3f76d781e3ce50329c0243ee0f00c0fcfb186c81df50e64bfdd34970148113f8ade90887f53e3c4938183afba830b4ba8e30a35 + languageName: node + linkType: hard + "@vitejs/plugin-react@npm:^1.3.0": version: 1.3.2 resolution: "@vitejs/plugin-react@npm:1.3.2" @@ -4766,6 +4847,7 @@ __metadata: eslint: ^8.14.0 eslint-config-prettier: ^8.5.0 eslint-plugin-import: ^2.27.5 + eslint-plugin-jest: ^27.2.2 eslint-plugin-prettier: ^4.0.0 express: ^4.18.2 get-monorepo-packages: ^1.2.0 @@ -8113,6 +8195,24 @@ __metadata: languageName: node linkType: hard +"eslint-plugin-jest@npm:^27.2.2": + version: 27.2.2 + resolution: "eslint-plugin-jest@npm:27.2.2" + dependencies: + "@typescript-eslint/utils": ^5.10.0 + peerDependencies: + "@typescript-eslint/eslint-plugin": ^5.0.0 + eslint: ^7.0.0 || ^8.0.0 + jest: "*" + peerDependenciesMeta: + "@typescript-eslint/eslint-plugin": + optional: true + jest: + optional: true + checksum: 98b63252d985f5dedf36ce9587dd4a0d24daf71ca8a997258343402c0d33ddd5070502378dafd9ac7fc0ef2e0d557b5c77f18e09ad73c71a52de8061db88293f + languageName: node + linkType: hard + "eslint-plugin-jsx-a11y@npm:^6.5.1": version: 6.5.1 resolution: "eslint-plugin-jsx-a11y@npm:6.5.1" @@ -15436,6 +15536,17 @@ __metadata: languageName: node linkType: hard +"semver@npm:^7.3.7": + version: 7.5.4 + resolution: "semver@npm:7.5.4" + dependencies: + lru-cache: ^6.0.0 + bin: + semver: bin/semver.js + checksum: 12d8ad952fa353b0995bf180cdac205a4068b759a140e5d3c608317098b3575ac2f1e09182206bf2eb26120e1c0ed8fb92c48c592f6099680de56bb071423ca3 + languageName: node + linkType: hard + "send@npm:0.18.0": version: 0.18.0 resolution: "send@npm:0.18.0"