Skip to content

Commit

Permalink
core(lint): Add custom no-focused-tests and no-skipped-tests rules (
Browse files Browse the repository at this point in the history
#13461)

Adds two simple custom rules to ignore
`(it|test|describe).(skip|only)`. These rules now also flag vitest-based
`skip` and `only` functions but led to duplications with the two rules
from `eslint-plugin-jest`. So this patch also disables the jest versions in
favour of the custom rules. To be clear, the custom rules are likely a
bit less robust than the jest/vitest version but until we can use the
actual vitest plugin, I think it's fine to stay with our custom version.
  • Loading branch information
Lms24 authored Aug 29, 2024
1 parent 780992d commit 03d67c9
Show file tree
Hide file tree
Showing 6 changed files with 71 additions and 20 deletions.
2 changes: 1 addition & 1 deletion packages/core/test/lib/transports/offline.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 () => {
Expand Down
20 changes: 2 additions & 18 deletions packages/eslint-config-sdk/src/base.js
Original file line number Diff line number Diff line change
Expand Up @@ -184,24 +184,8 @@ module.exports = {
'@sentry-internal/sdk/no-optional-chaining': 'off',
'@sentry-internal/sdk/no-nullish-coalescing': 'off',
'@typescript-eslint/no-floating-promises': 'off',
},
},
{
// 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',
'@sentry-internal/sdk/no-focused-tests': 'error',
'@sentry-internal/sdk/no-skipped-tests': 'error',
},
},
{
Expand Down
2 changes: 2 additions & 0 deletions packages/eslint-plugin-sdk/src/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -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'),
},
};
32 changes: 32 additions & 0 deletions packages/eslint-plugin-sdk/src/rules/no-focused-tests.js
Original file line number Diff line number Diff line change
@@ -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.",
});
}
},
};
},
};
33 changes: 33 additions & 0 deletions packages/eslint-plugin-sdk/src/rules/no-skipped-tests.js
Original file line number Diff line number Diff line change
@@ -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.",
});
}
},
};
},
};
2 changes: 1 addition & 1 deletion packages/profiling-node/test/cpu_profiler.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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() {
Expand Down

0 comments on commit 03d67c9

Please sign in to comment.