⚡ Improved assert news performance (#75)

<!--
Copyright (C) 2020-2022 Arm Limited or its affiliates and Contributors.
All rights reserved.
SPDX-License-Identifier: Apache-2.0
-->
### Description

Improved `assert news` performance by first looking for news file in the
current commit before exploring the whole branch.


### Test Coverage

<!--
Please put an `x` in the correct box e.g. `[x]` to indicate the testing
coverage of this change.
-->

- [x]  This change is covered by existing or additional automated tests.
- [ ] Manual testing has been performed (and evidence provided) as
automated testing was not feasible.
- [ ] Additional tests are not required for this change (e.g.
documentation update).

Author: acabarbaye

.secrets.baseline

@@ -116,7 +116,7 @@
         "filename": "continuous_delivery_scripts/utils/git_helpers.py",
         "hashed_secret": "9af3842326eadff7d85a55edfec67f6d8a3a259f",
         "is_verified": false,
-        "line_number": 49
+        "line_number": 48
       }
     ],
     "tests/git_helper/htmlcov/status.json": [
@@ -425,5 +425,5 @@
       }
     ]
   },
-  "generated_at": "2022-12-22T00:31:43Z"
+  "generated_at": "2023-02-28T00:31:43Z"
 }

continuous_delivery_scripts/assert_news.py

@@ -5,10 +5,10 @@
 """Checks if valid news files are created for changes in the project."""
 import argparse
 import logging
+import pathlib
 import re
 import sys
 from typing import List, Union
-import pathlib
 
 from continuous_delivery_scripts.utils.configuration import configuration, ConfigurationVariable
 from continuous_delivery_scripts.utils.git_helpers import ProjectTempClone, LocalProjectRepository, GitWrapper
@@ -71,10 +71,15 @@ def find_news_files(git: GitWrapper, root_dir: str, news_dir: str) -> List[str]:
     Returns:
         list: list of absolute paths to news files
     """
-    files_changed = git.list_files_added_on_current_branch()
+    files_changed = git.list_files_added_to_current_commit()
+    # To speed up the process, we first look at files added to the current commit.
+    # If no news files were added, then we check for addition on the branch.
     # Relies on the fact GitWrapper returns paths that are always relative
     # to the project root.
     added_news_files = [file_path for file_path in files_changed if file_path.startswith(news_dir)]
+    if len(added_news_files) == 0:
+        files_changed = git.list_files_added_on_current_branch()
+        added_news_files = [file_path for file_path in files_changed if file_path.startswith(news_dir)]
     return [str(pathlib.Path(root_dir, file_path)) for file_path in added_news_files]
 
 

continuous_delivery_scripts/utils/git_helpers.py

@@ -7,11 +7,10 @@
 import os
 import re
 import shutil
-from pathlib import Path
-from typing import Optional, List, Union, Any, Tuple
-
 from git import Repo, Actor, GitCommandError
 from packaging import version
+from pathlib import Path
+from typing import Optional, List, Union, Any, Tuple
 
 from .configuration import configuration, ConfigurationVariable
 from .filesystem_helpers import TemporaryDirectory
@@ -401,7 +400,13 @@ def remote_branch_exists(self, branch_name: str) -> bool:
         return self.get_remote_branch(branch_name) is not None
 
     def _get_specific_changes(self, change_type: Optional[str], commit1: Any, commit2: Any) -> List[str]:
-        diff = commit1.diff(commit2)
+        diff = None
+        if commit1:
+            diff = commit1.diff(commit2) if commit2 else commit1.diff()
+        elif commit2:
+            diff = commit2.diff()
+        if not diff:
+            return []
         if change_type:
             change_type = change_type.upper()
             change_type = change_type if change_type in diff.change_type else None
@@ -547,6 +552,14 @@ def _get_remote(self) -> Optional[Any]:
             logger.warning(e)
             return None
 
+    def list_files_added_to_current_commit(self) -> List[str]:
+        """Returns a list of files added in the current commit."""
+        current_commit = self.repo.head.commit
+        previous_commit = self.repo.commit("HEAD~1")
+        if not current_commit:
+            current_commit = self.get_current_commit()
+        return self.get_changes_list(previous_commit, current_commit, change_type="a")
+
     def list_files_added_on_current_branch(self) -> List[str]:
         """Returns a list of files changed against master branch."""
         master_branch = self.get_master_branch()
@@ -565,8 +578,7 @@ def list_files_added_on_current_branch(self) -> List[str]:
                 # The branch point off `beta` is more recent than off `master`.
                 # Hence, the difference between current and `beta` should be considered.
                 branch_point = beta_branch_point
-        changes = self.get_changes_list(branch_point, current_branch_commit, change_type="a")
-        return changes
+        return self.get_changes_list(branch_point, current_branch_commit, change_type="a")
 
     def is_current_branch_feature(self) -> bool:
         """Returns boolean indicating if current branch is considered a feature."""

news/20230228103345.bugfix

@@ -0,0 +1 @@
+:zap: Improved the performance of `assert news` for cases where news files are added to the current commit

tests/assert_news/test_news_files_validation.py

@@ -13,6 +13,31 @@ class TestFindNewsFiles(TestCase):
     def test_returns_newly_added_news_files(self):
         """Given added files in git, it returns absolute paths to added news files."""
         fake_git_wrapper = mock.Mock(spec_set=GitWrapper)
+        fake_git_wrapper.list_files_added_to_current_commit.return_value = [
+            "foo/bar.py",
+            "news/1234.txt",
+            "news/wat.html",
+        ]
+        fake_git_wrapper.list_files_added_on_current_branch.return_value = [
+            "foo/bar.py",
+            "news/1234.txt",
+            "news/wat.html",
+        ]
+        news_dir = "news/"
+        root_dir = "/root/"
+
+        subject = find_news_files(git=fake_git_wrapper, root_dir=root_dir, news_dir=news_dir)
+
+        fake_git_wrapper.list_files_added_to_current_commit.assert_called_once()
+        fake_git_wrapper.list_files_added_on_current_branch.assert_not_called()
+        self.assertEqual(
+            subject, [str(pathlib.Path(root_dir, "news/1234.txt")), str(pathlib.Path(root_dir, "news/wat.html"))]
+        )
+
+    def test_returns_previously_added_news_files(self):
+        """Given added files in git, it returns absolute paths to added news files."""
+        fake_git_wrapper = mock.Mock(spec_set=GitWrapper)
+        fake_git_wrapper.list_files_added_to_current_commit.return_value = []
         fake_git_wrapper.list_files_added_on_current_branch.return_value = [
             "foo/bar.py",
             "news/1234.txt",
@@ -23,6 +48,8 @@ def test_returns_newly_added_news_files(self):
 
         subject = find_news_files(git=fake_git_wrapper, root_dir=root_dir, news_dir=news_dir)
 
+        fake_git_wrapper.list_files_added_to_current_commit.assert_called_once()
+        fake_git_wrapper.list_files_added_on_current_branch.assert_called_once()
         self.assertEqual(
             subject, [str(pathlib.Path(root_dir, "news/1234.txt")), str(pathlib.Path(root_dir, "news/wat.html"))]
         )

tests/git_helper/test_git_helpers.py

@@ -115,8 +115,10 @@ def test_file_addition(self):
             clone.commit("Test commit")
             self.assertNotEqual(previous_hash, clone.get_commit_hash())
             self.assertEqual(previous_count + 1, clone.get_commit_count())
-            added_files = [Path(clone.root).joinpath(f) for f in clone.list_files_added_on_current_branch()]
-            self.assertTrue(test_file in added_files)
+            added_files_to_commit = [Path(clone.root).joinpath(f) for f in clone.list_files_added_to_current_commit()]
+            self.assertTrue(test_file in added_files_to_commit)
+            added_files_to_branch = [Path(clone.root).joinpath(f) for f in clone.list_files_added_on_current_branch()]
+            self.assertTrue(test_file in added_files_to_branch)
 
     def test_repo_clean(self):
         """Test basic git clean on the clone."""
@@ -166,5 +168,7 @@ def test_file_addition_with_paths_to_initial_repository(self):
             self.assertTrue(clone.get_corresponding_path(test_file) in uncommitted_changes)
             clone.add(test_file)
             clone.commit("Test commit")
+            added_files_to_commit = [Path(git.root).joinpath(f) for f in clone.list_files_added_to_current_commit()]
             added_files = [Path(git.root).joinpath(f) for f in clone.list_files_added_on_current_branch()]
         self.assertTrue(test_file in added_files)
+        self.assertTrue(test_file in added_files_to_commit)