Renovate/conventional changelog angular 7.x #3725

silversonicaxel · 2023-10-25T14:55:09Z

Description

Improved unit tests section for migration to conventional-changelog-angular 7.0

Motivation and Context

#3698

Usage examples

/

How Has This Been Tested?

run yarn test locally

Types of changes

Fixed some unit tests related to conventional-changelog-angular
Improved data unit tests suite of scope-enum, but two tests still failing, I miss some overall logic about it.

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

My change requires a change to the documentation.
I have updated the documentation accordingly.
I have added tests to cover my changes.
All new and existing tests passed.

escapedcat · 2023-10-26T07:53:00Z

Tests are failing. Did these work locally for you?

silversonicaxel · 2023-10-26T08:40:55Z

In scope-enum, these tests still failing, I miss some overall logic about it.
First of all, are these failing tests correctly written in their expectations?
I ask you cos it is not super clear the context to me, and this makes it difficult to understand what to fix.

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

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

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

escapedcat · 2023-10-26T08:51:43Z

First of all, are these failing tests correctly written in their expectations?

tbh, I don't know as well. But in the past those never had issues.

The only hint I have is to dig into the commits and see when those were introduced and see if the commit-message(s) and context help. I.e. if those were introduced to check for a certain bug.

silversonicaxel · 2023-10-26T12:25:38Z

Please update me if you discover something. I would do the same. Thanks.

silversonicaxel · 2023-10-30T14:09:49Z

Do you believe @escapedcat the problem lives in parse ?

escapedcat · 2023-10-30T14:35:09Z

Do you believe @escapedcat the problem lives in parse ?

Hm, sorry, I don't know. I would need to dig into it as well. Currently I don't have time for that.

joberstein · 2023-11-11T18:14:35Z

@commitlint/rules/src/scope-enum.test.ts

@@ -97,18 +100,29 @@ test('scope-enum with empty scope and never should succeed empty enum', async ()

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


I think this test should expect false, similar to the new test below (the message must not have any of the forbidden scopes - and in this case, it does, indicating a validation failure).

this test currently passes with true expectations.
having it false will make it a failing test.

This looks like it was originally expecting false per the diff though, and is now failing.

joberstein · 2023-11-11T18:29:51Z

First of all, are these failing tests correctly written in their expectations?

tbh, I don't know as well. But in the past those never had issues.

The only hint I have is to dig into the commits and see when those were introduced and see if the commit-message(s) and context help. I.e. if those were introduced to check for a certain bug.

The last two tests (of the three that are failing) do seem incorrect to me also in terms of the result they expect given the test description - the current logic indicates that the validation should fail for 'never' only if all of the given scopes are included in the commit message. However, it seems more reasonable to me that one would want the validation to fail for 'never' only if any of the given scopes are included.

Example:

Message: "fix(a,b): Test"
Forbidden scopes: ['a', 'c']

Current logic:

Result: Pass (forbidden scopes does not include every message scope - forbidden scopes does not include 'b')

Proposed logic:

Result: Fail (message scope includes 'a', which is one of the forbidden scopes)

There's a subtle distinction here between !every (all elements must not) and !some (any element must not); the current logic is relying on !every.

Maybe can be addressed as a followup instead of in this task since it's a logic change?

silversonicaxel · 2023-11-14T22:55:05Z

hello @joberstein , my goal was to try to speed up this commitlint PR, but I miss a lot of context of the project as a whole and I do not have so much time to dive into the reason why.
if you have suggestion on how to solve the tests please let me know. Or if you prefer to comment them out to proceed with the release. But I'm wondering, feeling is that some logic in the parse package is not 100% correct. Anyway. What's your proposal?

joberstein · 2023-11-16T01:24:33Z

@commitlint/rules/src/scope-enum.test.ts

+		'bar',
+		'qux',
+	]);
+	const expected = false;


This test should return true per the current logic (a false result will be provided only if both 'bar' and 'baz' are in the scopes, but only 'bar' is in the scopes). This is because the current logic says: 'return the opposite of whether all message scopes - bar, baz - are included in the given scopes - bar, qux'. Both 'bar' and 'baz' are not in message scopes, and since the result evaluates to the opposite of that, the expectation should be true.

Given that, the current logic does seem incorrect, but I suggest making a new PR to update logic and test.

joberstein · 2023-11-16T02:15:06Z

@commitlint/rules/src/scope-enum.test.ts

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


Similar to the above, this test should return true per the current logic (a false result will be provided only if both 'bar' and 'baz' are in the scopes, but only 'bar' is in the scopes). This is because the current logic says: 'return the opposite of whether all message scopes - bar, baz - are included in the given scopes - bar'. Both 'bar' and 'baz' are not in message scopes, and since the result evaluates to the opposite of that, the expectation should be true.

joberstein · 2023-11-16T02:21:50Z

hello @joberstein , my goal was to try to speed up this commitlint PR, but I miss a lot of context of the project as a whole and I do not have so much time to dive into the reason why. if you have suggestion on how to solve the tests please let me know. Or if you prefer to comment them out to proceed with the release. But I'm wondering, feeling is that some logic in the parse package is not 100% correct. Anyway. What's your proposal?

I added a comment to each of the failing test and why the expectations are reversed, causing the tests to fail.

I agree that the parse package logic appears to be incorrect. If addressed in a separate PR, I imagine the logic should be:

For 'always', the validation should pass only if the message includes all the given scopes. Otherwise, it should fail. (currently implemented)
For 'never', the validation should fail if the message includes any of the given scopes. Otherwise, it should pass.

The problem here is that the 'never' case currently evaluates to the opposite of the 'always' case right now, but that is not equivalent to what I've proposed above. @escapedcat does that seem like it should be addressed as an issue?

escapedcat · 2023-11-16T03:11:57Z

Thanks for your research and proposals @joberstein !
If this logic was kinda wrong the whole time we might could address this in a separate issue/PR. I assume this would be a breaking change because some users might rely on the current logic?

joberstein · 2023-11-16T04:32:08Z

Thanks for your research and proposals @joberstein ! If this logic was kinda wrong the whole time we might could address this in a separate issue/PR. I assume this would be a breaking change because some users might rely on the current logic?

Sure, and makes sense to me as a breaking change if I'm understanding the original intent of the rule correctly.

silversonicaxel · 2023-11-16T08:09:33Z

Thank everyone for the support

escapedcat · 2023-11-16T10:20:46Z

Sorry, so, just for a recap to understand this better.
This PR is now improving the tests to and gets v7 to work without any breaking changes? Right?

Next steps could be:

create PR to "fix" wrong tests (will be a breaking change)
merge v7 into master (will be breaking change because it's different to v6?)
publish new major version

Does this sound right?

silversonicaxel · 2023-11-16T11:25:12Z

@escapedcat this PR indeed improve tests and get v7 up and running without breaking changes.

Next steps might be a bit more:

create PR to fix the parse package that seems corrupted as @joberstein was mentioning here Renovate/conventional changelog angular 7.x #3725 (comment) , including wrong tests that can be spread a bit here a bit there.
merge v7 into master.
publish new major version.

It could be a breaking change. Or a fix. When fix is done, it will be more clear if Breaking or not.

joberstein · 2023-11-17T01:00:07Z

@commitlint/rules/src/scope-enum.test.ts

 	const expected = true;
 	expect(actual).toEqual(expected);
 });

-test('scope-enum with multiple scopes should error on message with superfluous scope', async () => {
+test('scope-enum with multiple scope should succeed on message with superfluous scope', async () => {


I think the message was technically correct before in terms of test intent, even though it doesn't describe the assertion in the test. This should be failing and it's not because the behavior is incorrect.

joberstein

Approved, but @silversonicaxel I left a comment about the test description change, not sure how important it is.

joberstein · 2023-11-18T02:30:46Z

@commitlint/rules/src/scope-enum.test.ts

 });

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


This test description should say it 'should error' I believe.

…ate/conventional-changelog-angular-7.x

escapedcat · 2023-11-18T07:30:47Z

Thanks @silversonicaxel and @joberstein for this! Will merge

* chore: update dependency conventional-changelog-angular to v7 * Renovate/conventional changelog angular 7.x (#3725) * chore: update dependency conventional-changelog-angular to v7 * test: update conventional-changelog-angular unit tests * test: improve scope-enum unit tests --------- Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> --------- Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> Co-authored-by: Alessandro Rabitti <silversonicaxel@users.noreply.github.com>

silversonicaxel mentioned this pull request Oct 25, 2023

fix: angular conventional-changelog-angular depdendency in @commitlint/parse/package.json #3698

Closed

4 tasks

silversonicaxel force-pushed the renovate/conventional-changelog-angular-7.x branch from 89d64a9 to 785669f Compare October 28, 2023 12:51

silversonicaxel force-pushed the renovate/conventional-changelog-angular-7.x branch from 785669f to e77fbea Compare November 1, 2023 13:14

renovate bot force-pushed the renovate/conventional-changelog-angular-7.x branch from 0d52fda to 17ce0e1 Compare November 10, 2023 05:11

joberstein reviewed Nov 11, 2023

View reviewed changes

joberstein reviewed Nov 16, 2023

View reviewed changes

silversonicaxel force-pushed the renovate/conventional-changelog-angular-7.x branch from 0c60c9a to 8fb26ae Compare November 16, 2023 07:56

renovate bot and others added 2 commits November 16, 2023 09:04

chore: update dependency conventional-changelog-angular to v7

4f9f98b

fix: update conventional-changelog-angular unit tests

41dbee7

silversonicaxel force-pushed the renovate/conventional-changelog-angular-7.x branch from 228a38d to d2f8a46 Compare November 16, 2023 08:04

silversonicaxel force-pushed the renovate/conventional-changelog-angular-7.x branch 2 times, most recently from 967b191 to 76b25fb Compare November 16, 2023 08:34

silversonicaxel requested a review from joberstein November 16, 2023 09:33

joberstein reviewed Nov 17, 2023

View reviewed changes

joberstein approved these changes Nov 17, 2023

View reviewed changes

joberstein mentioned this pull request Nov 17, 2023

fix: Message scope validation for forbidden scopes is incorrect #3779

Closed

4 tasks

silversonicaxel force-pushed the renovate/conventional-changelog-angular-7.x branch from 3c9c48c to fd1f1d3 Compare November 17, 2023 09:10

joberstein reviewed Nov 18, 2023

View reviewed changes

fix: improve scope-enum unit tests

9fcfb05

silversonicaxel force-pushed the renovate/conventional-changelog-angular-7.x branch from 657c548 to 9fcfb05 Compare November 18, 2023 07:08

Merge branch 'renovate/conventional-changelog-angular-7.x' into renov…

da8fe35

…ate/conventional-changelog-angular-7.x

escapedcat merged commit 128f5f6 into conventional-changelog:renovate/conventional-changelog-angular-7.x Nov 18, 2023
4 checks passed

silversonicaxel mentioned this pull request Nov 20, 2023

fix: @commitlint/parse package logic appears incorrect #3784

Closed

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Renovate/conventional changelog angular 7.x #3725

Renovate/conventional changelog angular 7.x #3725

silversonicaxel commented Oct 25, 2023 •

edited

Loading

escapedcat commented Oct 26, 2023

silversonicaxel commented Oct 26, 2023

escapedcat commented Oct 26, 2023

silversonicaxel commented Oct 26, 2023

silversonicaxel commented Oct 30, 2023

escapedcat commented Oct 30, 2023

joberstein Nov 11, 2023

silversonicaxel Nov 14, 2023

joberstein Nov 16, 2023

joberstein commented Nov 11, 2023 •

edited

Loading

silversonicaxel commented Nov 14, 2023

joberstein Nov 16, 2023 •

edited

Loading

silversonicaxel Nov 16, 2023

joberstein Nov 16, 2023

silversonicaxel Nov 16, 2023

joberstein commented Nov 16, 2023

escapedcat commented Nov 16, 2023

joberstein commented Nov 16, 2023

silversonicaxel commented Nov 16, 2023

escapedcat commented Nov 16, 2023

silversonicaxel commented Nov 16, 2023

joberstein Nov 17, 2023

silversonicaxel Nov 17, 2023

joberstein left a comment

joberstein Nov 18, 2023

escapedcat commented Nov 18, 2023

Renovate/conventional changelog angular 7.x #3725

Renovate/conventional changelog angular 7.x #3725

Conversation

silversonicaxel commented Oct 25, 2023 • edited Loading

Description

Motivation and Context

Usage examples

How Has This Been Tested?

Types of changes

Checklist:

escapedcat commented Oct 26, 2023

silversonicaxel commented Oct 26, 2023

escapedcat commented Oct 26, 2023

silversonicaxel commented Oct 26, 2023

silversonicaxel commented Oct 30, 2023

escapedcat commented Oct 30, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joberstein commented Nov 11, 2023 • edited Loading

silversonicaxel commented Nov 14, 2023

joberstein Nov 16, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joberstein commented Nov 16, 2023

escapedcat commented Nov 16, 2023

joberstein commented Nov 16, 2023

silversonicaxel commented Nov 16, 2023

escapedcat commented Nov 16, 2023

silversonicaxel commented Nov 16, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joberstein left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

escapedcat commented Nov 18, 2023

silversonicaxel commented Oct 25, 2023 •

edited

Loading

joberstein commented Nov 11, 2023 •

edited

Loading

joberstein Nov 16, 2023 •

edited

Loading