From f521d59041afee6a8a82206b3871960409aaa612 Mon Sep 17 00:00:00 2001 From: Joey Vagedes Date: Tue, 1 Aug 2023 08:48:22 -0700 Subject: [PATCH] path_utilities: report invalid paths correctly (#364) on Edk2Path instantiation (and error_on_invalid_pp=True), an error would be logged for each invalid path, but the actual exception raised would report the last path checked, which could be a valid path. This commit updates the exception to report all invalid pps. --- edk2toollib/uefi/edk2/path_utilities.py | 9 +- tests.unit/test_path_utilities.py | 105 ++++++++++++++---------- 2 files changed, 65 insertions(+), 49 deletions(-) diff --git a/edk2toollib/uefi/edk2/path_utilities.py b/edk2toollib/uefi/edk2/path_utilities.py index fe4d5004..b3671332 100644 --- a/edk2toollib/uefi/edk2/path_utilities.py +++ b/edk2toollib/uefi/edk2/path_utilities.py @@ -85,20 +85,19 @@ def __init__(self, ws: os.PathLike, package_path_list: Iterable[os.PathLike], # assume current working dir relative. Will catch invalid dir when checking whole list candidate_package_path_list.append(Path(os.getcwd(), a)) - error = False + invalid_pp = [] for a in candidate_package_path_list[:]: if not a.is_dir(): self.logger.log(logging.ERROR if error_on_invalid_pp else logging.WARNING, f"Invalid package path entry {a.resolve()}") candidate_package_path_list.remove(a) - error = True + invalid_pp.append(str(a.resolve())) self.PackagePathList = [str(p) for p in candidate_package_path_list] - if error and error_on_invalid_pp: - raise NotADirectoryError(errno.ENOENT, os.strerror(errno.ENOENT), - a.resolve()) + if invalid_pp and error_on_invalid_pp: + raise NotADirectoryError(errno.ENOENT, os.strerror(errno.ENOENT), invalid_pp) # # Nested package check - ensure packages do not exist in a linear diff --git a/tests.unit/test_path_utilities.py b/tests.unit/test_path_utilities.py index 75fff816..0839c106 100644 --- a/tests.unit/test_path_utilities.py +++ b/tests.unit/test_path_utilities.py @@ -7,25 +7,26 @@ # SPDX-License-Identifier: BSD-2-Clause-Patent ## -import unittest import os +import shutil import sys import tempfile -import shutil -from edk2toollib.uefi.edk2.path_utilities import Edk2Path +import unittest from pathlib import Path, PurePath +from edk2toollib.uefi.edk2.path_utilities import Edk2Path + class PathUtilitiesTest(unittest.TestCase): def _make_file_helper(self, parentpath: str, filename: str) -> None: - ''' simple internal helper to make a file and write basic content''' + '''Simple internal helper to make a file and write basic content.''' with open(os.path.join(parentpath, filename), "w") as f: f.write("Unit test content") def _make_edk2_module_helper(self, parentpath: str, modname: str, extension_case_lower: bool = True) -> str: - ''' simple internal helper to make a folder and inf like edk2 - return the path to the module folder + '''Simple internal helper to make a folder and inf like edk2 + return the path to the module folder. ''' modfolder = os.path.join(parentpath, modname) os.makedirs(modfolder, exist_ok=True) @@ -34,7 +35,7 @@ def _make_edk2_module_helper(self, parentpath: str, modname: str, extension_case return modfolder def _make_edk2_package_helper(self, path: str, packagename: str, extension_case_lower: bool = True) -> str: - ''' simple internal helper to make an ed2 package as follows: + '''Simple internal helper to make an ed2 package as follows: path/ packagename/ @@ -60,15 +61,17 @@ def _make_edk2_package_helper(self, path: str, packagename: str, extension_case_ return pkgfolder def setUp(self): - ''' unittest fixture run before each test. - Create a tmpdir and set the current working dir there''' + '''Unittest fixture run before each test. + Create a tmpdir and set the current working dir there. + ''' self.tmp = tempfile.mkdtemp() self.precwd = os.getcwd() os.chdir(self.tmp) # move to tempfile root def tearDown(self): - ''' unittest fixture run after each test. - Delete the tempdir and set the current working dir back''' + '''Unittest fixture run after each test. + Delete the tempdir and set the current working dir back. + ''' os.chdir(self.precwd) if os.path.isdir(self.tmp): shutil.rmtree(self.tmp) @@ -76,12 +79,12 @@ def tearDown(self): # TESTS def test_basic_init_ws_abs(self): - ''' test edk2path with valid absolute path to workspace''' + '''Test edk2path with valid absolute path to workspace.''' pathobj = Edk2Path(self.tmp, []) self.assertEqual(pathobj.WorkspacePath, self.tmp) def test_basic_init_ws_cwd(self): - ''' test edk2path with a relative path to workspace''' + '''Test edk2path with a relative path to workspace.''' relpath = "testrootfolder" fullpath = os.path.join(self.tmp, relpath) os.mkdir(fullpath) @@ -89,14 +92,13 @@ def test_basic_init_ws_cwd(self): self.assertEqual(pathobj.WorkspacePath, fullpath) def test_nonexistant_ws(self): - ''' test edk2path with invalid workspace''' - + '''Test edk2path with invalid workspace.''' invalid_ws = os.path.join(self.tmp, "invalidpath") with self.assertRaises(Exception): Edk2Path(invalid_ws, []) def test_nonexistant_abs(self): - ''' test edk2path with valid ws but invalid pp''' + '''Test edk2path with valid ws but invalid pp.''' pp = "doesnot_exist" pp_full_1 = os.path.join(self.tmp, pp) # absolute path @@ -110,12 +112,12 @@ def test_nonexistant_abs(self): self.assertEqual(len(pathobj.PackagePathList), 0) def test_pp_inside_workspace(self): - ''' test with packagespath pointing to folder nested inside workspace + '''Test with packagespath pointing to folder nested inside workspace root/ <-- current working directory folder_ws/ <-- workspace root folder_pp/ <-- packages path pp packages here - ws packages here + ws packages here. ''' ws_rel = "folder_ws" ws_abs = os.path.join(self.tmp, ws_rel) @@ -140,12 +142,12 @@ def test_pp_inside_workspace(self): self.assertEqual(pathobj.PackagePathList[0], folder_pp1_abs) def test_pp_outside_workspace(self): - ''' test with packagespath pointing to folder outside of workspace + '''Test with packagespath pointing to folder outside of workspace root/ <-- current working directory folder_ws/ <-- workspace root ws packages here folder_pp/ <-- packages path - pp packages here + pp packages here. ''' ws_rel = "folder_ws" @@ -165,6 +167,21 @@ def test_pp_outside_workspace(self): self.assertEqual(pathobj.WorkspacePath, ws_abs) self.assertEqual(pathobj.PackagePathList[0], folder_pp1_abs) + def test_invalid_pp(self): + """Tests that the exception message contains all invalid paths and no valid paths.""" + ws = Path(self.tmp, "folder_ws") + ws.mkdir() + (ws / "good_path").mkdir() + + with self.assertRaises(NotADirectoryError) as context: + Edk2Path(ws, ["bad_pp_path", "bad_pp_path2", "good_path"], error_on_invalid_pp=True) + self.assertTrue('bad_pp_path' in str(context.exception)) + self.assertTrue('bad_pp_path2' in str(context.exception)) + self.assertTrue('good_path' not in str(context.exception)) + + # Make sure we don't throw an exception unless we mean to + Edk2Path(ws, ["bad_pp_path", "bad_pp_path2", "good_path"], error_on_invalid_pp=False) + @unittest.skipUnless(sys.platform.startswith("win"), "requires Windows") def test_basic_init_ws_abs_different_case(self): inputPath = self.tmp.capitalize() @@ -175,8 +192,8 @@ def test_basic_init_ws_abs_different_case(self): self.assertNotEqual(pathobj.WorkspacePath, self.tmp) def test_get_containing_package_inside_workspace(self): - ''' test basic usage of GetContainingPackage with packages path nested - inside the workspace + '''Test basic usage of GetContainingPackage with packages path nested + inside the workspace. File layout: @@ -248,8 +265,8 @@ def test_get_containing_package_inside_workspace(self): self.assertIsNone(pathobj.GetContainingPackage(p)) def test_get_containing_package_outside_workspace(self): - ''' test basic usage of GetContainingPackage with packages path - outside the workspace + '''Test basic usage of GetContainingPackage with packages path + outside the workspace. File layout: @@ -315,7 +332,7 @@ def test_get_containing_package_outside_workspace(self): @unittest.skipUnless(sys.platform.startswith("win"), "requires Windows") def test_get_containing_package_ws_abs_different_case(self): - ''' test basic usage of GetContainingPackage when the workspace path has different case for + '''Test basic usage of GetContainingPackage when the workspace path has different case for the drive letter then the incoming paths. This can happen on Windows if os.path.realpath is used. @@ -411,7 +428,7 @@ def test_get_containing_modules_with_relative_path(self): module2.inf X64 TestFile.c - """ + """ # Make the workspace directory: folder_ws/ ws_rel = "folder_ws" ws_abs = os.path.join(self.tmp, ws_rel) @@ -451,7 +468,7 @@ def test_get_containing_modules_with_relative_path(self): self.assertRaises(Exception, pathobj.GetContainingModules, p) def test_get_containing_module_with_infs_in_other_temp_dirs(self): - ''' test that GetContainingModule does not look outside the workspace + '''Test that GetContainingModule does not look outside the workspace root for modules. To do so, a temporary .inf file is placed in the user's temporary directory. Such a file could already exist and similarly impact test results. To ensure consistent test results, this @@ -562,8 +579,8 @@ def test_get_containing_modules_path_format(self): self.assertEqual(expected_module_inf, actual_module_inf_list[0]) def test_get_containing_module(self): - ''' test basic usage of GetContainingModule with packages path nested - inside the workspace + '''Test basic usage of GetContainingModule with packages path nested + inside the workspace. File layout: @@ -742,8 +759,8 @@ def test_get_edk2_relative_path_from_absolute_path_posix(self): self.assertEqual(expected_rel_from_abs_path, actual_rel_from_abs_path) def test_get_edk2_relative_path_from_absolute_path(self): - ''' test basic usage of GetEdk2RelativePathFromAbsolutePath with packages path nested - inside the workspace + '''Test basic usage of GetEdk2RelativePathFromAbsolutePath with packages path nested + inside the workspace. File layout: @@ -815,7 +832,7 @@ def test_get_edk2_relative_path_from_absolute_path(self): self.assertEqual(pathobj.GetEdk2RelativePathFromAbsolutePath(p), f"{ws_p_name}/Module2/x64/TESTFILE.C") def test_get_relative_path_when_packages_path_list_contains_substrings(self): - ''' test usage of GetEdk2RelativePathFromAbsolutePath when members of PackagePathList contain + '''Test usage of GetEdk2RelativePathFromAbsolutePath when members of PackagePathList contain substrings of themselves, for example "MU" and "MU_TIANO" File layout: root/ <-- current working directory (self.tmp) @@ -833,7 +850,7 @@ def test_get_relative_path_when_packages_path_list_contains_substrings(self): module2/ module2.INF X64/ - TestFile2.c + TestFile2.c. ''' ws_rel = "folder_ws" ws_abs = os.path.join(self.tmp, ws_rel) @@ -853,7 +870,7 @@ def test_get_relative_path_when_packages_path_list_contains_substrings(self): self.assertEqual(pathobj.GetEdk2RelativePathFromAbsolutePath(p), f"{ws_p_name}/module2/X64/TestFile2.c") def test_get_relative_path_when_package_path_inside_package(self): - ''' test a package_path directory inside the subfolders of a package. + '''Test a package_path directory inside the subfolders of a package. Should not raise an exception. File layout: @@ -890,7 +907,7 @@ def test_get_relative_path_when_package_path_inside_package(self): self.assertEqual(pathobj.GetEdk2RelativePathFromAbsolutePath(p), f"{pp1_name}/module2/X64/TestFile.c") def test_get_relative_path_with_nested_packages(self): - ''' test a two package paths that contain nested packages. + '''Test a two package paths that contain nested packages. Should raise an exception due to nested packages. File layout: @@ -965,7 +982,7 @@ def test_get_relative_path_with_nested_packages(self): os.environ.pop("PYTOOL_IGNORE_KNOWN_BAD_NESTED_PACKAGES") def test_get_relative_path_when_folder_is_next_to_package(self): - ''' test usage of GetEdk2RelativePathFromAbsolutePath when a folder containing a package is in the same + '''Test usage of GetEdk2RelativePathFromAbsolutePath when a folder containing a package is in the same directory as a different package. This test ensures the correct value is returned regardless the order of the package paths. file layout: @@ -974,7 +991,7 @@ def test_get_relative_path_when_folder_is_next_to_package(self): folder_pp1/ <-- A Package Path PPTestPkg1 <-- A Package folder_pp2/ <-- A Package Path - PPTestPkg2 <-- A Package + PPTestPkg2 <-- A Package. ''' folder_ws_rel = "folder_ws" folder_ws_abs = os.path.join(self.tmp, folder_ws_rel) @@ -1005,8 +1022,8 @@ def test_get_relative_path_when_folder_is_next_to_package(self): self.assertEqual(pathobj.GetEdk2RelativePathFromAbsolutePath(p2), f'{pp2_name}/module2/X64/TestFile2.c') def test_get_relative_path_when_path_does_not_exist(self): - ''' test basic usage of GetEdk2RelativePathFromAbsolutePath with packages path nested - inside the workspace, but the absolute path is not a real path + '''Test basic usage of GetEdk2RelativePathFromAbsolutePath with packages path nested + inside the workspace, but the absolute path is not a real path. File layout: @@ -1067,7 +1084,7 @@ def test_get_relative_path_when_path_does_not_exist(self): self.assertIsNone(pathobj.GetEdk2RelativePathFromAbsolutePath(p)) def test_get_relative_path_when_package_is_not_directly_inside_packages_path(self): - ''' test basic usage of GetEdk2RelativePathFromAbsolutePath when the + '''Test basic usage of GetEdk2RelativePathFromAbsolutePath when the package is not a direct subfolder of a packagespath, but atleast one folder away. @@ -1101,8 +1118,8 @@ def test_get_relative_path_when_package_is_not_directly_inside_packages_path(sel f"{folder_extra_rel}/{ws_p_name}/{ws_p_name}.dec") def test_get_absolute_path_on_this_system_from_edk2_relative_path(self): - ''' test basic usage of GetAbsolutePathOnThisSystemFromEdk2RelativePath with packages path nested - inside the workspace + '''Test basic usage of GetAbsolutePathOnThisSystemFromEdk2RelativePath with packages path nested + inside the workspace. File layout: @@ -1156,7 +1173,7 @@ def test_get_absolute_path_on_this_system_from_edk2_relative_path(self): self.assertIsNone(pathobj.GetAbsolutePathOnThisSystemFromEdk2RelativePath(None)) def test_get_absolute_path_then_relative_path_when_path_contains_repeated_packagepath_name(self): - ''' Test the back and forth between GetAbsolutePath and GetRelativeFromAbsolute when the + '''Test the back and forth between GetAbsolutePath and GetRelativeFromAbsolute when the path structure has multiple instances of a package path File layout: root/ <-- current working directory (self.tmp) @@ -1169,7 +1186,7 @@ def test_get_absolute_path_then_relative_path_when_path_contains_repeated_packag module2/ module2.INF x64/ - TestFile.c + TestFile.c. ''' ws_rel = "PlatformClientPkg" ws_abs = os.path.join(self.tmp, ws_rel)