Fix bug introduced to `updateChangelog` in #158 by MajorLift · Pull Request #181 · MetaMask/auto-changelog

MajorLift · 2023-12-08T03:01:05Z

Motivation

#158 incorrectly refactored the branches in updateChangelog, resulting in a state where new changelog entries weren't added to the return value if isReleaseCandidate is false.

Explanation

This error is fixed by moving the logic for retrieving new entries out of the if (isReleaseCandidate) block.
The code for retrieving new entries is also extracted into its own method: getNewChangeEntries, abstracting away details that obscured the flow of the method's core logic.
To verify that the buggy logic is correctly restored, it's necessary to compare the current state of updateChangelog to its state before the bug was introduced. For this, see: diff link.

References

- `updateChangelog` returns results even if `isReleaseCandidate` is false

MajorLift · 2024-02-08T07:08:45Z

src/update-changelog.ts

+  const newChangelogContent = changelog.toString();
+  const isChangelogUpdated = changelogContent !== newChangelogContent;
+  return isChangelogUpdated ? newChangelogContent : undefined;


update() in cli.ts expects that updateChangelog return undefined if there are no updates to be added to the changelog.

MajorLift · 2024-02-08T07:10:26Z

src/update-changelog.ts

-  if (
-    newCommits.length === 0 &&
-    (!isReleaseCandidate || hasUnreleasedChanges)
-  ) {
-    return undefined;
-  }


Previously, the undefined return branch was located here.

MajorLift · 2024-02-08T07:11:16Z

src/update-changelog.ts

-  const commitRange =
-    mostRecentTag === null ? 'HEAD' : `${mostRecentTag}..HEAD`;
-  const commitsHashesSinceLastRelease = await getCommitHashesInRange(
-    commitRange,
-    projectRootDirectory,
-  );
-  const commits = await getCommits(commitsHashesSinceLastRelease);
-
-  const loggedPrNumbers = getAllLoggedPrNumbers(changelog);
-  const newCommits = commits.filter(
-    ({ prNumber }) =>
-      prNumber === undefined || !loggedPrNumbers.includes(prNumber),
-  );


This section is moved to getNewChangeEntries().

MajorLift · 2024-02-08T07:11:29Z

src/update-changelog.ts

-    const newChangeEntries = newCommits.map(({ prNumber, description }) => {
-      if (prNumber) {
-        const suffix = `([#${prNumber}](${repoUrl}/pull/${prNumber}))`;
-        return `${description} ${suffix}`;
-      }
-      return description;
-    });


This section is moved to getNewChangeEntries().

MajorLift · 2024-02-08T07:12:24Z

src/update-changelog.ts

  });

-  // Ensure we have all tags on remote
-  await runCommand('git', ['fetch', '--tags']);


This line is moved to getMostRecentTag().

MajorLift · 2024-02-08T07:14:28Z

src/update-changelog.ts

    }
-
-    if (mostRecentTag === `${tagPrefixes[0]}${currentVersion ?? ''}`) {
+    if (mostRecentTag === `${tagPrefixes[0]}${currentVersion}`) {


currentVersion is narrowed to string thanks to the if (!currentVersion) block being moved directly above. Previously as string or ?? '' was required.

MajorLift · 2024-02-08T07:16:24Z

src/update-changelog.ts

@@ -264,28 +289,31 @@ export async function updateChangelog({
      changelog.addRelease({ version: currentVersion });


currentVersion is correctly narrowed due to all related branches being refactored to fall under the if (isReleaseCandidate) block instead of there being multiple if (isReleaseCandidate &&) blocks. currentVersion as Version casting is now unnecessary.

MajorLift · 2024-02-08T07:18:24Z

src/update-changelog.ts

+      category: ChangeCategory.Uncategorized,
+      description,
+    });
  }


This is the bracket that caused the bug. Moving it to above the getNewChangeEntries call resolves the error.

kanthesha

Overall looks good. Few minor suggestions.

kanthesha · 2024-02-08T12:18:05Z

src/update-changelog.ts

+  projectRootDirectory,
+}: AddNewCommitsOptions) {
+  const commitRange =
+    mostRecentTag === null ? 'HEAD' : `${mostRecentTag}..HEAD`;


is there a possibility to be little more flexible here!

Suggested change

mostRecentTag === null ? 'HEAD' : `${mostRecentTag}..HEAD`;

!mostRecentTag ? 'HEAD' : `${mostRecentTag}..HEAD`;

Thanks for pointing these out! Unfortunately, this one results in a linter error:

Unexpected negated condition. eslint(no-negated-condition)

How about this?

Suggested change

mostRecentTag === null ? 'HEAD' : `${mostRecentTag}..HEAD`;

mostRecentTag ? `${mostRecentTag}..HEAD` : 'HEAD';

That would work, but I would prefer to prioritize type safety over brevity here, since mostRecentTag is derived from the output of running an actual git command in shell. There are no strong guarantees on what the runtime output will be, so we can't completely rule out edge cases where e.g. an empty string or undefined is returned.

This probably aligns with the original motivation for getMostRecentTag returning null in its early exit branch. It represents the exception case where we know for sure that no tag exists, which is distinct from the error case where some tags exist, but the most recent tag could not be found due to a failure or edge case. This second case would be better represented by an undefined.

What are your thoughts? In general, I feel less comfortable replacing === null with !, because our codebase mostly uses undefined, which makes me assume that any null usage is intentional.

Hmm, we did use null intentionally to represent lack of a most recent tag, so the use of null matches the type that getMostRecentTag returns. However, I suppose that the tag name could be an empty string in theory if for some reason git describe returns an empty string, and that case we would want to fall back to HEAD. So, we could update getMostRecentTag to just return string instead of string | null, except that in case there is no recent tag, it would then return '';. That might look strange, but we could add a comment saying why we're doing that or give that empty string a name that describe its purpose.

In any case, it seems that this line was merely copied from another function, so maybe we want to make that change in another PR?

I like using null for clear distinction and visibility, but in any case I agree with revisiting this in its own ticket since this wasn't a change introduced by this PR.

src/update-changelog.ts

…fined

mcmire

Hopefully in writing the tests no bugs are revealed, but comparing this to the original version, this makes sense to me.

mcmire · 2024-02-08T19:13:10Z

src/update-changelog.ts

+  projectRootDirectory,
+}: AddNewCommitsOptions) {
+  const commitRange =
+    mostRecentTag === null ? 'HEAD' : `${mostRecentTag}..HEAD`;


Hmm, we did use null intentionally to represent lack of a most recent tag, so the use of null matches the type that getMostRecentTag returns. However, I suppose that the tag name could be an empty string in theory if for some reason git describe returns an empty string, and that case we would want to fall back to HEAD. So, we could update getMostRecentTag to just return string instead of string | null, except that in case there is no recent tag, it would then return '';. That might look strange, but we could add a comment saying why we're doing that or give that empty string a name that describe its purpose.

In any case, it seems that this line was merely copied from another function, so maybe we want to make that change in another PR?

mcmire · 2024-02-08T19:16:50Z

src/update-changelog.ts

    }
-
-    if (mostRecentTag === `${tagPrefixes[0]}${currentVersion ?? ''}`) {
+    if (mostRecentTag === `${tagPrefixes[0]}${currentVersion}`) {


Hmm, I just realized that we only use the first tag prefix here, which seems strange. This may be a potential bug. But we can investigate that later.

The jsdoc does specify that the first prefix is the intended prefix and the rest are fallbacks. parseChangelog also only uses the first prefix.

getMostRecentTag does iterate over the tagPrefix list. It should return the used prefix, or we could extract the prefix from the returned most recent tag.

But this also wasn't a change introduced in this PR, so it should get its own ticket.

MajorLift added bug Something isn't working team-wallet-framework labels Dec 8, 2023

MajorLift self-assigned this Dec 8, 2023

MajorLift force-pushed the 231207-update-changelog-fix-write-tests branch 3 times, most recently from d973245 to 4e8abf3 Compare December 14, 2023 21:39

MajorLift added 4 commits February 8, 2024 01:04

[update-changelog] Fix bug introduced in #158

3008846

- `updateChangelog` returns results even if `isReleaseCandidate` is false

[update-changelog] refactor: extract getNewChangeEntries method

8cec96e

Move git fetch --tags call to getMostRecentTag

d18d0fa

Rewrite return statement for readability

aeabb00

MajorLift force-pushed the 231207-update-changelog-fix-write-tests branch from 2103825 to 4cedf60 Compare February 8, 2024 06:23

MajorLift changed the title ~~Fix bug introduced to updateChangelog in #158 and write tests~~ Fix bug introduced to updateChangelog in #158 Feb 8, 2024

MajorLift force-pushed the 231207-update-changelog-fix-write-tests branch from 4cedf60 to aeabb00 Compare February 8, 2024 06:40

Replace Object.keys with getKnownPropertyNames

da39a58

MajorLift commented Feb 8, 2024

View reviewed changes

MajorLift marked this pull request as ready for review February 8, 2024 07:21

MajorLift requested a review from a team as a code owner February 8, 2024 07:21

MajorLift mentioned this pull request Feb 8, 2024

Set up testing environment for updateChangelog and write comprehensive unit tests #188

Open

kanthesha reviewed Feb 8, 2024

View reviewed changes

src/update-changelog.ts Outdated Show resolved Hide resolved

Apply suggestion to prefer negation over strict equals check for unde…

3aef0c9

…fined

MajorLift force-pushed the 231207-update-changelog-fix-write-tests branch from 5bd3d65 to 3aef0c9 Compare February 8, 2024 14:53

mcmire approved these changes Feb 8, 2024

View reviewed changes

MajorLift merged commit 5ebfe97 into main Feb 8, 2024

MajorLift deleted the 231207-update-changelog-fix-write-tests branch February 8, 2024 22:04

		@@ -264,28 +289,31 @@ export async function updateChangelog({
		changelog.addRelease({ version: currentVersion });

	mostRecentTag === null ? 'HEAD' : `${mostRecentTag}..HEAD`;
	!mostRecentTag ? 'HEAD' : `${mostRecentTag}..HEAD`;

	mostRecentTag === null ? 'HEAD' : `${mostRecentTag}..HEAD`;
	mostRecentTag ? `${mostRecentTag}..HEAD` : 'HEAD';

Uh oh!

Conversation

MajorLift commented Dec 8, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation

Explanation

References

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MajorLift Feb 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MajorLift Feb 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kanthesha left a comment

Choose a reason for hiding this comment

Uh oh!

kanthesha Feb 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MajorLift Feb 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MajorLift Feb 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mcmire left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

MajorLift commented Dec 8, 2023 •

edited

Loading

MajorLift Feb 8, 2024 •

edited

Loading

MajorLift Feb 8, 2024 •

edited

Loading

kanthesha Feb 8, 2024 •

edited

Loading

MajorLift Feb 8, 2024 •

edited

Loading

MajorLift Feb 8, 2024 •

edited

Loading