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.

Sign up for GitHub

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

Fix/3779: Adjust logic for 'never' scope-enum validation #3795

Merged
merged 11 commits into from
Dec 9, 2023
Merged

Fix/3779: Adjust logic for 'never' scope-enum validation #3795

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
2d567b3
feat(load): use cosmiconfig-typescript-loader v5 to remove ts-node de…
joberstein Oct 24, 2023
6dcec0c
Merge branch 'feat/bump-cosmiconfig-typescript-loader-2'
joberstein Oct 26, 2023
b9cd734
Merge branch 'conventional-changelog:master' into master
joberstein Oct 27, 2023
145ca9d
Merge branch 'conventional-changelog:master' into master
joberstein Nov 3, 2023
b8c0c00
Merge branch 'conventional-changelog:master' into master
joberstein Nov 8, 2023
2a545c5
Merge branch 'conventional-changelog:master' into master
joberstein Nov 9, 2023
7c7ce18
Merge branch 'conventional-changelog:master' into master
joberstein Nov 10, 2023
57ce694
Merge branch 'conventional-changelog:master' into master
joberstein Nov 16, 2023
235eef1
Merge branch 'conventional-changelog:master' into master
joberstein Nov 16, 2023
ee4ca6f
Merge branch 'conventional-changelog:master' into master
joberstein Nov 22, 2023
ac4e8e4
fix(load): adjust "never" logic for scope enum
joberstein Nov 22, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
312 changes: 178 additions & 134 deletions @commitlint/rules/src/scope-enum.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,139 +1,183 @@
import parse from '@commitlint/parse';
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks like a lot of changes, but in summary, I organized the old tests by use-case and they are still getting executed; in addition, I added more automated tests to cover all of the message types for the following cases for both always and never:

  • Enum is empty (always passes for both always and never)
  • Messages without scopes (always passes for both always and never)
  • Always - messages with one or multiple scopes
  • Never - messages with scopes

import {scopeEnum} from './scope-enum';

const messages = {
plain: 'foo(bar): baz',
superfluous: 'foo(): baz',
empty: 'foo: baz',
multiple: 'foo(bar,baz): qux',
multipleCommaSpace: 'foo(bar, baz): qux',
};

const parsed = {
plain: parse(messages.plain),
superfluous: parse(messages.superfluous),
empty: parse(messages.empty),
multiple: parse(messages.multiple),
multipleCommaSpace: parse(messages.multipleCommaSpace),
import {RuleConfigCondition} from '@commitlint/types';

const messagesByScope = {
single: {
plain: 'foo(bar): baz',
},
multiple: {
multiple: 'foo(bar,baz): qux',
multipleCommaSpace: 'foo(bar, baz): qux',
},
none: {
empty: 'foo: baz',
superfluous: 'foo(): baz',
},
};

test('scope-enum with plain message and always should succeed empty enum', async () => {
const [actual] = scopeEnum(await parsed.plain, 'always', []);
const expected = true;
expect(actual).toEqual(expected);
});

test('scope-enum with plain message and never should error empty enum', async () => {
const [actual, message] = scopeEnum(await parsed.plain, 'never', []);
const expected = false;
expect(actual).toEqual(expected);
expect(message).toEqual('scope must not be one of []');
});

test('with plain message should succeed correct enum', async () => {
const [actual] = scopeEnum(await parsed.plain, 'always', ['bar']);
const expected = true;
expect(actual).toEqual(expected);
});

test('scope-enum with plain message should error false enum', async () => {
const [actual, message] = scopeEnum(await parsed.plain, 'always', ['foo']);
const expected = false;
expect(actual).toEqual(expected);
expect(message).toEqual('scope must be one of [foo]');
});

test('scope-enum with plain message should error forbidden enum', async () => {
const [actual, message] = scopeEnum(await parsed.plain, 'never', ['bar']);
const expected = false;
expect(actual).toEqual(expected);
expect(message).toEqual('scope must not be one of [bar]');
});

test('scope-enum with plain message should succeed forbidden enum', async () => {
const [actual] = scopeEnum(await parsed.plain, 'never', ['foo']);
const expected = true;
expect(actual).toEqual(expected);
});

test('scope-enum with superfluous scope should succeed enum', async () => {
const [actual] = scopeEnum(await parsed.superfluous, 'always', ['bar']);
const expected = true;
expect(actual).toEqual(expected);
});

test('scope-enum with superfluous scope and "never" should succeed', async () => {
const [actual] = scopeEnum(await parsed.superfluous, 'never', ['bar']);
const expected = true;
expect(actual).toEqual(expected);
});

test('scope-enum with superfluous scope and always should succeed empty enum', async () => {
const [actual] = scopeEnum(await parsed.superfluous, 'always', []);
const expected = true;
expect(actual).toEqual(expected);
});

test('scope-enum with superfluous scope and never should succeed empty enum', async () => {
const [actual] = scopeEnum(await parsed.superfluous, 'never', []);
const expected = true;
expect(actual).toEqual(expected);
});

test('scope-enum with empty scope and always should succeed empty enum', async () => {
const [actual] = scopeEnum(await parsed.superfluous, 'always', []);
const expected = true;
expect(actual).toEqual(expected);
});

test('scope-enum with empty scope and always should succeed filled enum', async () => {
const [actual] = scopeEnum(await parsed.superfluous, 'always', ['foo']);
const expected = true;
expect(actual).toEqual(expected);
});

test('scope-enum with empty scope and never should succeed empty enum', async () => {
const [actual] = scopeEnum(await parsed.superfluous, 'never', []);
const expected = true;
expect(actual).toEqual(expected);
});

test('scope-enum with multiple scopes should error on message with multiple scopes', async () => {
const [actual, message] = scopeEnum(await parsed.multiple, 'never', [
'bar',
'baz',
]);
const expected = false;
expect(actual).toEqual(expected);
expect(message).toEqual('scope must not be one of [bar, baz]');
});

test('scope-enum with multiple scopes should error on message with forbidden enum', async () => {
const [actual, message] = scopeEnum(await parsed.multiple, 'never', [
'bar',
'qux',
]);
const expected = true;
expect(actual).toEqual(expected);
});

test('scope-enum with multiple scopes should error on message with superfluous scope', async () => {
const [actual] = scopeEnum(await parsed.multiple, 'never', ['bar']);
const expected = true;
expect(actual).toEqual(expected);
});

test('scope-enum with multiple scope should succeed on message with multiple scopes', async () => {
const [actual] = scopeEnum(await parsed.multiple, 'always', ['bar', 'baz']);
const expected = true;
expect(actual).toEqual(expected);
});

test('scope-enum with multiple scope with comma+space should succeed on message with multiple scopes', async () => {
const [actual] = scopeEnum(await parsed.multipleCommaSpace, 'always', [
'bar',
'baz',
]);
const expected = true;
expect(actual).toEqual(expected);
const {single, multiple, none} = messagesByScope;

const messages = Object.values(messagesByScope).reduce(
(acc, curr) => ({...acc, ...curr}),
{}
);

const conditions: RuleConfigCondition[] = ['always', 'never'];

describe('Scope Enum Validation', () => {
conditions.forEach((condition) => {
describe('Enum without Scopes', () => {
Object.keys(messages).forEach((messageType) => {
test(`Succeeds with a '${messageType}' message and '${condition}'`, async () => {
const [actual, message] = scopeEnum(
await parse(messages[messageType]),
condition,
[]
);
const expected = true;
expect(actual).toEqual(expected);
expect(message).toEqual('');
});
});
});

describe('Messages without Scopes', () => {
Object.keys(none).forEach((messageType) => {
const fakeMessage = messages[messageType];

it(`Succeeds with a '${messageType}' message and '${condition}' with single-scope enum`, async () => {
const [actual, message] = scopeEnum(
await parse(fakeMessage),
condition,
['bar']
);
expect(actual).toBeTruthy();
expect(message).toBeFalsy();
});

it(`Succeeds with a '${messageType}' message and '${condition}' with multi-scope enum`, async () => {
const [actual, message] = scopeEnum(
await parse(fakeMessage),
condition,
['bar', 'baz']
);
expect(actual).toBeTruthy();
expect(message).toBeFalsy();
});
});
});
});

describe('Always', () => {
describe('Single-Scope Messages', () => {
Object.keys(single).forEach((messageType) => {
const fakeMessage = messages[messageType];

it(`Succeeds with a '${messageType}' message when all message scopes are included in single-scope enum`, async () => {
const [actual, message] = scopeEnum(
await parse(fakeMessage),
'always',
['bar']
);
expect(actual).toBeTruthy();
expect(message).toEqual('scope must be one of [bar]');
});

test(`Succeeds with a '${messageType}' message when all message scopes are included in multi-scope enum`, async () => {
const [actual, message] = scopeEnum(
await parse(fakeMessage),
'always',
['bar', 'baz']
);
expect(actual).toBeTruthy();
expect(message).toEqual('scope must be one of [bar, baz]');
});

test(`Fails with a '${messageType}' message when any message scope is not included in enum`, async () => {
const [actual, message] = scopeEnum(
await parse(fakeMessage),
'always',
['foo']
);
expect(actual).toBeFalsy();
expect(message).toEqual('scope must be one of [foo]');
});
});
});

describe('Multi-Scope Messages', () => {
Object.keys(multiple).forEach((messageType) => {
const fakeMessage = messages[messageType];

test(`Succeeds with a '${messageType}' message when all message scopes are included in enum`, async () => {
const [actual, message] = scopeEnum(
await parse(fakeMessage),
'always',
['bar', 'baz']
);
expect(actual).toBeTruthy();
expect(message).toEqual('scope must be one of [bar, baz]');
});

test(`Fails with a '${messageType}' message when no message scopes are included in enum`, async () => {
const [actual, message] = scopeEnum(
await parse(fakeMessage),
'always',
['foo']
);
expect(actual).toBeFalsy();
expect(message).toEqual('scope must be one of [foo]');
});

it(`Fails with a '${messageType}' message when only some message scopes are included in enum`, async () => {
const [actual, message] = scopeEnum(
await parse(fakeMessage),
'always',
['bar']
);
expect(actual).toBeFalsy();
expect(message).toEqual('scope must be one of [bar]');
});
});
});
});

describe('Never', () => {
describe('Messages with Scopes', () => {
Object.keys({...single, ...multiple}).forEach((messageType) => {
const fakeMessage = messages[messageType];

test(`Succeeds with a '${messageType}' message when no message scopes are included in enum`, async () => {
const [actual, message] = scopeEnum(
await parse(fakeMessage),
'never',
['foo']
);
expect(actual).toBeTruthy();
expect(message).toEqual('scope must not be one of [foo]');
});

it(`Fails with a '${messageType}' message when any message scope is included in single-scope enum`, async () => {
const [actual, message] = scopeEnum(
await parse(fakeMessage),
'never',
['bar']
);
expect(actual).toBeFalsy();
expect(message).toEqual('scope must not be one of [bar]');
});

test(`Fails with a '${messageType}' message when any message scope is included in multi-scope enum`, async () => {
const [actual, message] = scopeEnum(
await parse(fakeMessage),
'never',
['bar', 'baz']
);
expect(actual).toBeFalsy();
expect(message).toEqual('scope must not be one of [bar, baz]');
});
});
});
});
});
28 changes: 13 additions & 15 deletions @commitlint/rules/src/scope-enum.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,30 +3,28 @@ import message from '@commitlint/message';
import {SyncRule} from '@commitlint/types';

export const scopeEnum: SyncRule<string[]> = (
parsed,
{scope},
when = 'always',
value = []
) => {
if (!parsed.scope) {
if (!scope || !value.length) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The value.length check was further down before; I can't reason out a case where "scopeEnum(..., 'never', [])" should fail, which was the original behavior, but just wanted to call this out.

return [true, ''];
}

// Scopes may contain slash or comma delimiters to separate them and mark them as individual segments.
// This means that each of these segments should be tested separately with `ensure`.
const delimiters = /\/|\\|, ?/g;
const scopeSegments = parsed.scope.split(delimiters);
const messageScopes = scope.split(delimiters);
const errorMessage = ['scope must', `be one of [${value.join(', ')}]`];
const isScopeInEnum = (scope: string) => ensure.enum(scope, value);
let isValid;

const negated = when === 'never';
const result =
value.length === 0 ||
scopeSegments.every((scope) => ensure.enum(scope, value));
if (when === 'never') {
isValid = !messageScopes.some(isScopeInEnum);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Main logic change, essentially from "!messageScopes.every(isScopeInEnum)"

errorMessage.splice(1, 0, 'not');
} else {
isValid = messageScopes.every(isScopeInEnum);
}

return [
negated ? !result : result,
message([
`scope must`,
negated ? `not` : null,
`be one of [${value.join(', ')}]`,
]),
];
return [isValid, message(errorMessage)];
};
18 changes: 17 additions & 1 deletion docs/reference-rules.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -225,6 +225,10 @@ Infinity
```
[]
```
- Notes:
- This rule always passes if no scopes are provided in the message or the value is an empty array.
- When set to `always`, all message scopes must be found in the value.
- When set to `never`, none of the message scopes can be found in the value.

#### scope-case

Expand Down Expand Up @@ -347,7 +351,19 @@ Infinity
- **rule**: `always`
- **value**
```js
['build', 'chore', 'ci', 'docs', 'feat', 'fix', 'perf', 'refactor', 'revert', 'style', 'test'];
[
'build',
'chore',
'ci',
'docs',
'feat',
'fix',
'perf',
'refactor',
'revert',
'style',
'test',
];
```

#### type-case
Expand Down