From b7ff0daad2d04679ee3d4a399a286793c7eece24 Mon Sep 17 00:00:00 2001 From: Lukas Stracke Date: Mon, 26 Aug 2024 16:15:54 +0200 Subject: [PATCH 1/2] core(lint): Add custom `no-focused-tests` and `no-skipped-tests` rules --- packages/eslint-config-sdk/src/base.js | 2 ++ packages/eslint-plugin-sdk/src/index.js | 2 ++ .../src/rules/no-focused-tests.js | 32 ++++++++++++++++++ .../src/rules/no-skipped-tests.js | 33 +++++++++++++++++++ 4 files changed, 69 insertions(+) create mode 100644 packages/eslint-plugin-sdk/src/rules/no-focused-tests.js create mode 100644 packages/eslint-plugin-sdk/src/rules/no-skipped-tests.js diff --git a/packages/eslint-config-sdk/src/base.js b/packages/eslint-config-sdk/src/base.js index 9a6fa807e09f..30e20d8b5e6b 100644 --- a/packages/eslint-config-sdk/src/base.js +++ b/packages/eslint-config-sdk/src/base.js @@ -184,6 +184,8 @@ module.exports = { '@sentry-internal/sdk/no-optional-chaining': 'off', '@sentry-internal/sdk/no-nullish-coalescing': 'off', '@typescript-eslint/no-floating-promises': 'off', + '@sentry-internal/sdk/no-focused-tests': 'error', + '@sentry-internal/sdk/no-skipped-tests': 'error', }, }, { diff --git a/packages/eslint-plugin-sdk/src/index.js b/packages/eslint-plugin-sdk/src/index.js index 4390af285609..d7516c343d60 100644 --- a/packages/eslint-plugin-sdk/src/index.js +++ b/packages/eslint-plugin-sdk/src/index.js @@ -15,5 +15,7 @@ module.exports = { 'no-eq-empty': require('./rules/no-eq-empty'), 'no-class-field-initializers': require('./rules/no-class-field-initializers'), 'no-regexp-constructor': require('./rules/no-regexp-constructor'), + 'no-focused-tests': require('./rules/no-focused-tests'), + 'no-skipped-tests': require('./rules/no-skipped-tests'), }, }; diff --git a/packages/eslint-plugin-sdk/src/rules/no-focused-tests.js b/packages/eslint-plugin-sdk/src/rules/no-focused-tests.js new file mode 100644 index 000000000000..008431780da2 --- /dev/null +++ b/packages/eslint-plugin-sdk/src/rules/no-focused-tests.js @@ -0,0 +1,32 @@ +'use strict'; + +/** + * This rule was created to flag usages of the `.only` function in vitest and jest tests. + * Usually, we don't want to commit focused tests as this causes other tests to be skipped. + */ +module.exports = { + meta: { + docs: { + description: "Do not focus tests via `.only` to ensure we don't commit accidentally skip the other tests.", + }, + schema: [], + }, + create: function (context) { + return { + CallExpression(node) { + if ( + node.callee.type === 'MemberExpression' && + node.callee.object.type === 'Identifier' && + ['test', 'it', 'describe'].includes(node.callee.object.name) && + node.callee.property.type === 'Identifier' && + node.callee.property.name === 'only' + ) { + context.report({ + node, + message: "Do not focus tests via `.only` to ensure we don't commit accidentally skip the other tests.", + }); + } + }, + }; + }, +}; diff --git a/packages/eslint-plugin-sdk/src/rules/no-skipped-tests.js b/packages/eslint-plugin-sdk/src/rules/no-skipped-tests.js new file mode 100644 index 000000000000..2c11e6e071a0 --- /dev/null +++ b/packages/eslint-plugin-sdk/src/rules/no-skipped-tests.js @@ -0,0 +1,33 @@ +'use strict'; + +/** + * This rule was created to flag usages of the `.skip` function in vitest and jest tests. + * Usually, we don't want to commit skipped tests as this causes other tests to be skipped. + * Sometimes, skipping is valid (e.g. flaky tests), in which case, we can simply eslint-disable the rule. + */ +module.exports = { + meta: { + docs: { + description: "Do not skip tests via `.skip` to ensure we don't commit accidentally skipped tests.", + }, + schema: [], + }, + create: function (context) { + return { + CallExpression(node) { + if ( + node.callee.type === 'MemberExpression' && + node.callee.object.type === 'Identifier' && + ['test', 'it', 'describe'].includes(node.callee.object.name) && + node.callee.property.type === 'Identifier' && + node.callee.property.name === 'skip' + ) { + context.report({ + node, + message: "Do not skip tests via `.skip` to ensure we don't commit accidentally skipped tests.", + }); + } + }, + }; + }, +}; From faa749efa70d795c0e0223cf62c5d4014be71283 Mon Sep 17 00:00:00 2001 From: Lukas Stracke Date: Mon, 26 Aug 2024 16:28:49 +0200 Subject: [PATCH 2/2] replace jest eslint rules --- .../core/test/lib/transports/offline.test.ts | 2 +- packages/eslint-config-sdk/src/base.js | 18 ------------------ .../profiling-node/test/cpu_profiler.test.ts | 2 +- 3 files changed, 2 insertions(+), 20 deletions(-) diff --git a/packages/core/test/lib/transports/offline.test.ts b/packages/core/test/lib/transports/offline.test.ts index ea3fa13ffd69..738597197178 100644 --- a/packages/core/test/lib/transports/offline.test.ts +++ b/packages/core/test/lib/transports/offline.test.ts @@ -410,7 +410,7 @@ describe('makeOfflineTransport', () => { START_DELAY + 2_000, ); - // eslint-disable-next-line jest/no-disabled-tests + // eslint-disable-next-line @sentry-internal/sdk/no-skipped-tests it.skip( 'Follows the Retry-After header', async () => { diff --git a/packages/eslint-config-sdk/src/base.js b/packages/eslint-config-sdk/src/base.js index 30e20d8b5e6b..6daa79eaeed8 100644 --- a/packages/eslint-config-sdk/src/base.js +++ b/packages/eslint-config-sdk/src/base.js @@ -188,24 +188,6 @@ module.exports = { '@sentry-internal/sdk/no-skipped-tests': 'error', }, }, - { - // Configuration only for test files (this won't apply to utils or other files in test directories) - plugins: ['jest'], - env: { - jest: true, - }, - files: ['test.ts', '*.test.ts', '*.test.tsx', '*.test.js', '*.test.jsx'], - rules: { - // Prevent permanent usage of `it.only`, `fit`, `test.only` etc - // We want to avoid debugging leftovers making their way into the codebase - 'jest/no-focused-tests': 'error', - - // Prevent permanent usage of `it.skip`, `xit`, `test.skip` etc - // We want to avoid debugging leftovers making their way into the codebase - // If there's a good reason to skip a test (e.g. bad flakiness), just add an ignore comment - 'jest/no-disabled-tests': 'error', - }, - }, { // Configuration for config files like webpack/rollup files: ['*.config.js', '*.config.mjs'], diff --git a/packages/profiling-node/test/cpu_profiler.test.ts b/packages/profiling-node/test/cpu_profiler.test.ts index c1086003c1af..1e3903be6fc5 100644 --- a/packages/profiling-node/test/cpu_profiler.test.ts +++ b/packages/profiling-node/test/cpu_profiler.test.ts @@ -316,7 +316,7 @@ describe('Profiler bindings', () => { expect(profile?.measurements?.['memory_footprint']?.values.length).toBeLessThanOrEqual(300); }); - // eslint-disable-next-line jest/no-disabled-tests + // eslint-disable-next-line @sentry-internal/sdk/no-skipped-tests it.skip('includes deopt reason', async () => { // https://github.com/petkaantonov/bluebird/wiki/Optimization-killers#52-the-object-being-iterated-is-not-a-simple-enumerable function iterateOverLargeHashTable() {