From 51ed75827e47280ae23842b0f27dfa264e8b4b5c Mon Sep 17 00:00:00 2001 From: Qingyu Sui Date: Fri, 17 Feb 2023 18:15:04 +0800 Subject: [PATCH 1/5] Add unit tests for TarFile class (for #668) --- .gitignore | 1 + tests/tar/BUILD | 26 +++++++++++++ tests/tar/build_tar_test.py | 64 +++++++++++++++++++++++++++++++ tests/tar/helper.py | 45 ++++++++++++++++++++++ tests/tar/tar_writer_test.py | 67 ++++++--------------------------- tests/testdata/subdir/world.txt | 1 + 6 files changed, 149 insertions(+), 55 deletions(-) create mode 100644 tests/tar/build_tar_test.py create mode 100644 tests/tar/helper.py create mode 100644 tests/testdata/subdir/world.txt diff --git a/.gitignore b/.gitignore index 716b1b36..56a872c6 100644 --- a/.gitignore +++ b/.gitignore @@ -1,3 +1,4 @@ .bazeliskrc .ijwb bazel-* +__pycache__/ diff --git a/tests/tar/BUILD b/tests/tar/BUILD index 07111d43..c8c3eacf 100644 --- a/tests/tar/BUILD +++ b/tests/tar/BUILD @@ -28,6 +28,13 @@ package( default_visibility = ["//visibility:private"], ) +py_binary( + name = "helper", + srcs = ["helper.py"], + python_version = "PY3", + srcs_version = "PY3", +) + py_test( name = "tar_writer_test", srcs = [ @@ -47,11 +54,30 @@ py_test( python_version = "PY3", srcs_version = "PY3", deps = [ + ":helper", "//pkg/private/tar:tar_writer", "@bazel_tools//tools/python/runfiles", ], ) +py_test( + name = "build_tar_test", + srcs = [ + "build_tar_test.py", + ], + data = [ + "//tests:testdata/hello.txt", + "//tests:testdata/subdir/world.txt", + ], + python_version = "PY3", + srcs_version = "PY3", + deps = [ + ":helper", + "//pkg/private/tar:build_tar", + "@bazel_tools//tools/python/runfiles", + ], +) + genrule( name = "generate_files", outs = [ diff --git a/tests/tar/build_tar_test.py b/tests/tar/build_tar_test.py new file mode 100644 index 00000000..56a42233 --- /dev/null +++ b/tests/tar/build_tar_test.py @@ -0,0 +1,64 @@ +# Copyright 2015 The Bazel Authors. All rights reserved. +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. +"""Testing for build_tar.""" + +import os +import unittest + +from bazel_tools.tools.python.runfiles import runfiles +from pkg.private.tar import build_tar +from tests.tar import helper + + +class TarFileUnitTest(unittest.TestCase): + """Unit testing for TarFile class.""" + + def setUp(self): + super(TarFileUnitTest, self).setUp() + self.tempfile = os.path.join(os.environ["TEST_TMPDIR"], "test.tar") + self.data_files = runfiles.Create() + # Keep the trailing slash stripped. Add slash manually when needed. + self.directory = self.data_files.Rlocation("rules_pkg/tests/testdata/").strip('/') + + def tearDown(self): + super(TarFileUnitTest, self).tearDown() + if os.path.exists(self.tempfile): + os.remove(self.tempfile) + + def test_normalize_path(self): + path_without_leading_period = os.path.sep.join(("foo", "bar", "")) + path_with_leading_period = os.path.sep.join((".", "foo", "bar", "")) + with build_tar.TarFile(self.tempfile, self.directory, "", "", None) as tar_file_obj: + self.assertEqual(tar_file_obj.normalize_path(path_without_leading_period), self.directory + "/foo/bar") + self.assertEqual(tar_file_obj.normalize_path(path_with_leading_period), self.directory + "/foo/bar") + with build_tar.TarFile(self.tempfile, self.directory + "/", "", "", None) as tar_file_obj: + self.assertEqual(tar_file_obj.normalize_path(path_without_leading_period), self.directory + "/foo/bar") + self.assertEqual(tar_file_obj.normalize_path(path_with_leading_period), self.directory + "/foo/bar") + with build_tar.TarFile(self.tempfile, "/", "", "", None) as tar_file_obj: + self.assertEqual(tar_file_obj.normalize_path(path_without_leading_period), "foo/bar") + self.assertEqual(tar_file_obj.normalize_path(path_with_leading_period), "foo/bar") + + def test_add_tree(self): + with build_tar.TarFile(self.tempfile, "/", "", "", None) as tar_file_obj: + tar_file_obj.add_tree(self.data_files.Rlocation("rules_pkg/tests/testdata/"), "/") + helper.assertTarFileContent(self, self.tempfile, [ + {"name": "./hello.txt", "data": "Hello, world!\n".encode("utf-8")}, + {"name": "subdir"}, + {"name": "./subdir"}, + {"name": "./subdir/world.txt", "data": "Hello, world!\n".encode("utf-8")}, + ]) + + +if __name__ == "__main__": + unittest.main() diff --git a/tests/tar/helper.py b/tests/tar/helper.py new file mode 100644 index 00000000..04c22d01 --- /dev/null +++ b/tests/tar/helper.py @@ -0,0 +1,45 @@ +import os +import tarfile + +def assertTarFileContent(test_class, tar, content): + """Assert that tarfile contains exactly the entry described by `content`. + + Args: + tar: the path to the TAR file to test. + content: an array describing the expected content of the TAR file. + Each entry in that list should be a dictionary where each field + is a field to test in the corresponding TarInfo. For + testing the presence of a file "x", then the entry could simply + be `{"name": "x"}`, the missing field will be ignored. To match + the content of a file entry, use the key "data". + """ + got_names = [] + with tarfile.open(tar, "r:*") as f: + for current in f: + got_names.append(getattr(current, "name")) + + with tarfile.open(tar, "r:*") as f: + i = 0 + for current in f: + error_msg = "Extraneous file at end of archive %s: %s" % ( + tar, + current.name + ) + test_class.assertLess(i, len(content), error_msg) + for k, v in content[i].items(): + if k == "data": + value = f.extractfile(current).read() + elif k == "name" and os.name == "nt": + value = getattr(current, k).replace("\\", "/") + else: + value = getattr(current, k) + error_msg = " ".join([ + "Value `%s` for key `%s` of file" % (value, k), + "%s in archive %s does" % (current.name, tar), + "not match expected value `%s`" % v + ]) + error_msg += str(got_names) + test_class.assertEqual(value, v, error_msg) + i += 1 + if i < len(content): + test_class.fail("Missing file %s in archive %s" % (content[i], tar)) diff --git a/tests/tar/tar_writer_test.py b/tests/tar/tar_writer_test.py index 39008eae..5d4b3a0f 100644 --- a/tests/tar/tar_writer_test.py +++ b/tests/tar/tar_writer_test.py @@ -19,55 +19,12 @@ from bazel_tools.tools.python.runfiles import runfiles from pkg.private.tar import tar_writer -from tests.tar import compressor +from tests.tar import compressor, helper class TarFileWriterTest(unittest.TestCase): """Testing for TarFileWriter class.""" - def assertTarFileContent(self, tar, content): - """Assert that tarfile contains exactly the entry described by `content`. - - Args: - tar: the path to the TAR file to test. - content: an array describing the expected content of the TAR file. - Each entry in that list should be a dictionary where each field - is a field to test in the corresponding TarInfo. For - testing the presence of a file "x", then the entry could simply - be `{"name": "x"}`, the missing field will be ignored. To match - the content of a file entry, use the key "data". - """ - got_names = [] - with tarfile.open(tar, "r:*") as f: - for current in f: - got_names.append(getattr(current, "name")) - - with tarfile.open(tar, "r:*") as f: - i = 0 - for current in f: - error_msg = "Extraneous file at end of archive %s: %s" % ( - tar, - current.name - ) - self.assertLess(i, len(content), error_msg) - for k, v in content[i].items(): - if k == "data": - value = f.extractfile(current).read() - elif k == "name" and os.name == "nt": - value = getattr(current, k).replace("\\", "/") - else: - value = getattr(current, k) - error_msg = " ".join([ - "Value `%s` for key `%s` of file" % (value, k), - "%s in archive %s does" % (current.name, tar), - "not match expected value `%s`" % v - ]) - error_msg += str(got_names) - self.assertEqual(value, v, error_msg) - i += 1 - if i < len(content): - self.fail("Missing file %s in archive %s" % (content[i], tar)) - def setUp(self): super(TarFileWriterTest, self).setUp() self.tempfile = os.path.join(os.environ["TEST_TMPDIR"], "test.tar") @@ -81,7 +38,7 @@ def tearDown(self): def testEmptyTarFile(self): with tar_writer.TarFileWriter(self.tempfile): pass - self.assertTarFileContent(self.tempfile, []) + helper.assertTarFileContent(self, self.tempfile, []) def assertSimpleFileContent(self, names): with tar_writer.TarFileWriter(self.tempfile) as f: @@ -92,7 +49,7 @@ def assertSimpleFileContent(self, names): "size": len(n.encode("utf-8")), "data": n.encode("utf-8")} for n in names]) - self.assertTarFileContent(self.tempfile, content) + helper.assertTarFileContent(self, self.tempfile, content) def testAddFile(self): self.assertSimpleFileContent(["./a"]) @@ -118,7 +75,7 @@ def testDottedFiles(self): {"name": "..e"}, {"name": ".f"} ] - self.assertTarFileContent(self.tempfile, content) + helper.assertTarFileContent(self, self.tempfile, content) def testMergeTar(self): content = [ @@ -130,7 +87,7 @@ def testMergeTar(self): datafile = self.data_files.Rlocation( "rules_pkg/tests/testdata/tar_test.tar" + ext) f.add_tar(datafile, name_filter=lambda n: n != "./b") - self.assertTarFileContent(self.tempfile, content) + helper.assertTarFileContent(self, self.tempfile, content) def testMergeTarRelocated(self): content = [ @@ -142,7 +99,7 @@ def testMergeTarRelocated(self): datafile = self.data_files.Rlocation( "rules_pkg/tests/testdata/tar_test.tar") f.add_tar(datafile, name_filter=lambda n: n != "./b", prefix="foo") - self.assertTarFileContent(self.tempfile, content) + helper.assertTarFileContent(self, self.tempfile, content) def testDefaultMtimeNotProvided(self): with tar_writer.TarFileWriter(self.tempfile) as f: @@ -182,7 +139,7 @@ def testAddingDirectoriesForFile(self): {"name": "d", "mode": 0o755}, {"name": "d/f"}, ] - self.assertTarFileContent(self.tempfile, content) + helper.assertTarFileContent(self, self.tempfile, content) def testAddingDirectoriesForFileManually(self): with tar_writer.TarFileWriter(self.tempfile) as f: @@ -208,7 +165,7 @@ def testAddingDirectoriesForFileManually(self): {"name": "x/y", "mode": 0o755}, {"name": "x/y/f"}, ] - self.assertTarFileContent(self.tempfile, content) + helper.assertTarFileContent(self, self.tempfile, content) def testPackageDirAttribute(self): """Tests package_dir of pkg_tar.""" @@ -220,7 +177,7 @@ def testPackageDirAttribute(self): {"name": "my/package/mylink"}, {"name": "my/package/nsswitch.conf"}, ] - self.assertTarFileContent(package_dir, expected_content) + helper.assertTarFileContent(self, package_dir, expected_content) def testPackageDirFileAttribute(self): """Tests package_dir_file attributes of pkg_tar.""" @@ -230,7 +187,7 @@ def testPackageDirFileAttribute(self): {"name": "package"}, {"name": "package/nsswitch.conf"}, ] - self.assertTarFileContent(package_dir_file, expected_content) + helper.assertTarFileContent(self, package_dir_file, expected_content) def testCustomCompression(self): original = self.data_files.Rlocation( @@ -245,8 +202,8 @@ def testCustomCompression(self): expected_content = [ {"name": "./" + x, "data": x.encode("utf-8")} for x in ["a", "b", "ab"] ] - self.assertTarFileContent(original, expected_content) - self.assertTarFileContent(self.tempfile, expected_content) + helper.assertTarFileContent(self, original, expected_content) + helper.assertTarFileContent(self, self.tempfile, expected_content) if __name__ == "__main__": diff --git a/tests/testdata/subdir/world.txt b/tests/testdata/subdir/world.txt new file mode 100644 index 00000000..af5626b4 --- /dev/null +++ b/tests/testdata/subdir/world.txt @@ -0,0 +1 @@ +Hello, world! From 2cc49c03171e8c711eca6f5e430f8993da14a0c7 Mon Sep 17 00:00:00 2001 From: Qingyu Sui Date: Fri, 17 Feb 2023 18:30:06 +0800 Subject: [PATCH 2/5] Append the newly added text file info to the golden file of the glob for texts --- tests/mappings/glob_for_texts_manifest.golden | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/tests/mappings/glob_for_texts_manifest.golden b/tests/mappings/glob_for_texts_manifest.golden index c144b83f..ba78d730 100644 --- a/tests/mappings/glob_for_texts_manifest.golden +++ b/tests/mappings/glob_for_texts_manifest.golden @@ -2,5 +2,6 @@ {"type": "file", "dest": "file_with_a_ridiculously_long_name_consectetur_adipiscing_elit_fusce_laoreet_lorem_neque_sed_pharetra_erat.txt", "src": "tests/testdata/file_with_a_ridiculously_long_name_consectetur_adipiscing_elit_fusce_laoreet_lorem_neque_sed_pharetra_erat.txt", "mode": "", "user": null, "group": null, "origin": "@//tests:glob_for_texts"}, {"type": "file", "dest": "hello.txt", "src": "tests/testdata/hello.txt", "mode": "", "user": null, "group": null, "origin": "@//tests:glob_for_texts"}, {"type": "file", "dest": "loremipsum.txt", "src": "tests/testdata/loremipsum.txt", "mode": "", "user": null, "group": null, "origin": "@//tests:glob_for_texts"}, - {"type": "file", "dest": "test_tar_package_dir_file.txt", "src": "tests/testdata/test_tar_package_dir_file.txt", "mode": "", "user": null, "group": null, "origin": "@//tests:glob_for_texts"} + {"type": "file", "dest": "test_tar_package_dir_file.txt", "src": "tests/testdata/test_tar_package_dir_file.txt", "mode": "", "user": null, "group": null, "origin": "@//tests:glob_for_texts"}, + {"type": "file", "dest": "world.txt", "src": "tests/testdata/subdir/world.txt", "mode": "", "user": null, "group": null, "origin": "@//tests:glob_for_texts"} ] From b7daaab4c6227919a6d81d42d7aa418859c3fbd5 Mon Sep 17 00:00:00 2001 From: Qingyu Sui Date: Wed, 22 Feb 2023 13:30:15 +0800 Subject: [PATCH 3/5] Remove unnecessary tests which does not help exploiting the problem in #668 --- tests/mappings/glob_for_texts_manifest.golden | 3 +-- tests/tar/BUILD | 1 - tests/tar/build_tar_test.py | 16 ---------------- tests/testdata/subdir/world.txt | 1 - 4 files changed, 1 insertion(+), 20 deletions(-) delete mode 100644 tests/testdata/subdir/world.txt diff --git a/tests/mappings/glob_for_texts_manifest.golden b/tests/mappings/glob_for_texts_manifest.golden index ba78d730..c144b83f 100644 --- a/tests/mappings/glob_for_texts_manifest.golden +++ b/tests/mappings/glob_for_texts_manifest.golden @@ -2,6 +2,5 @@ {"type": "file", "dest": "file_with_a_ridiculously_long_name_consectetur_adipiscing_elit_fusce_laoreet_lorem_neque_sed_pharetra_erat.txt", "src": "tests/testdata/file_with_a_ridiculously_long_name_consectetur_adipiscing_elit_fusce_laoreet_lorem_neque_sed_pharetra_erat.txt", "mode": "", "user": null, "group": null, "origin": "@//tests:glob_for_texts"}, {"type": "file", "dest": "hello.txt", "src": "tests/testdata/hello.txt", "mode": "", "user": null, "group": null, "origin": "@//tests:glob_for_texts"}, {"type": "file", "dest": "loremipsum.txt", "src": "tests/testdata/loremipsum.txt", "mode": "", "user": null, "group": null, "origin": "@//tests:glob_for_texts"}, - {"type": "file", "dest": "test_tar_package_dir_file.txt", "src": "tests/testdata/test_tar_package_dir_file.txt", "mode": "", "user": null, "group": null, "origin": "@//tests:glob_for_texts"}, - {"type": "file", "dest": "world.txt", "src": "tests/testdata/subdir/world.txt", "mode": "", "user": null, "group": null, "origin": "@//tests:glob_for_texts"} + {"type": "file", "dest": "test_tar_package_dir_file.txt", "src": "tests/testdata/test_tar_package_dir_file.txt", "mode": "", "user": null, "group": null, "origin": "@//tests:glob_for_texts"} ] diff --git a/tests/tar/BUILD b/tests/tar/BUILD index c8c3eacf..005850bb 100644 --- a/tests/tar/BUILD +++ b/tests/tar/BUILD @@ -67,7 +67,6 @@ py_test( ], data = [ "//tests:testdata/hello.txt", - "//tests:testdata/subdir/world.txt", ], python_version = "PY3", srcs_version = "PY3", diff --git a/tests/tar/build_tar_test.py b/tests/tar/build_tar_test.py index 56a42233..d60ed62f 100644 --- a/tests/tar/build_tar_test.py +++ b/tests/tar/build_tar_test.py @@ -36,27 +36,11 @@ def tearDown(self): if os.path.exists(self.tempfile): os.remove(self.tempfile) - def test_normalize_path(self): - path_without_leading_period = os.path.sep.join(("foo", "bar", "")) - path_with_leading_period = os.path.sep.join((".", "foo", "bar", "")) - with build_tar.TarFile(self.tempfile, self.directory, "", "", None) as tar_file_obj: - self.assertEqual(tar_file_obj.normalize_path(path_without_leading_period), self.directory + "/foo/bar") - self.assertEqual(tar_file_obj.normalize_path(path_with_leading_period), self.directory + "/foo/bar") - with build_tar.TarFile(self.tempfile, self.directory + "/", "", "", None) as tar_file_obj: - self.assertEqual(tar_file_obj.normalize_path(path_without_leading_period), self.directory + "/foo/bar") - self.assertEqual(tar_file_obj.normalize_path(path_with_leading_period), self.directory + "/foo/bar") - with build_tar.TarFile(self.tempfile, "/", "", "", None) as tar_file_obj: - self.assertEqual(tar_file_obj.normalize_path(path_without_leading_period), "foo/bar") - self.assertEqual(tar_file_obj.normalize_path(path_with_leading_period), "foo/bar") - def test_add_tree(self): with build_tar.TarFile(self.tempfile, "/", "", "", None) as tar_file_obj: tar_file_obj.add_tree(self.data_files.Rlocation("rules_pkg/tests/testdata/"), "/") helper.assertTarFileContent(self, self.tempfile, [ {"name": "./hello.txt", "data": "Hello, world!\n".encode("utf-8")}, - {"name": "subdir"}, - {"name": "./subdir"}, - {"name": "./subdir/world.txt", "data": "Hello, world!\n".encode("utf-8")}, ]) diff --git a/tests/testdata/subdir/world.txt b/tests/testdata/subdir/world.txt deleted file mode 100644 index af5626b4..00000000 --- a/tests/testdata/subdir/world.txt +++ /dev/null @@ -1 +0,0 @@ -Hello, world! From ce6738e24bdccf95b6f7659547ba29daff095093 Mon Sep 17 00:00:00 2001 From: Qingyu Sui Date: Wed, 22 Feb 2023 14:05:54 +0800 Subject: [PATCH 4/5] Try fix Rlocation path not found error by finding file instead of directory --- tests/tar/build_tar_test.py | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/tests/tar/build_tar_test.py b/tests/tar/build_tar_test.py index d60ed62f..1974a155 100644 --- a/tests/tar/build_tar_test.py +++ b/tests/tar/build_tar_test.py @@ -28,8 +28,13 @@ def setUp(self): super(TarFileUnitTest, self).setUp() self.tempfile = os.path.join(os.environ["TEST_TMPDIR"], "test.tar") self.data_files = runfiles.Create() + test_file_name = "hello.txt" + self.directory = self.data_files.Rlocation("rules_pkg/tests/testdata/" + test_file_name) + # Remove the file name to get directory. + if (self.directory.endswith(test_file_name)): + self.directory = self.directory[:-len(test_file_name)] # Keep the trailing slash stripped. Add slash manually when needed. - self.directory = self.data_files.Rlocation("rules_pkg/tests/testdata/").strip('/') + self.directory = self.directory.rstrip("/") def tearDown(self): super(TarFileUnitTest, self).tearDown() @@ -38,7 +43,7 @@ def tearDown(self): def test_add_tree(self): with build_tar.TarFile(self.tempfile, "/", "", "", None) as tar_file_obj: - tar_file_obj.add_tree(self.data_files.Rlocation("rules_pkg/tests/testdata/"), "/") + tar_file_obj.add_tree(self.directory + "/", "/") helper.assertTarFileContent(self, self.tempfile, [ {"name": "./hello.txt", "data": "Hello, world!\n".encode("utf-8")}, ]) From 6f3309024f8a044078eb39a1db438426ad676f69 Mon Sep 17 00:00:00 2001 From: Qingyu Sui Date: Wed, 22 Feb 2023 14:25:26 +0800 Subject: [PATCH 5/5] Use build_tar subdir in testdata for add_tree test, because Bazel seems copying all testdata files when running in Windows --- tests/tar/BUILD | 2 +- tests/tar/build_tar_test.py | 6 +++--- tests/testdata/build_tar/world | 1 + 3 files changed, 5 insertions(+), 4 deletions(-) create mode 100644 tests/testdata/build_tar/world diff --git a/tests/tar/BUILD b/tests/tar/BUILD index 005850bb..7e5c421c 100644 --- a/tests/tar/BUILD +++ b/tests/tar/BUILD @@ -66,7 +66,7 @@ py_test( "build_tar_test.py", ], data = [ - "//tests:testdata/hello.txt", + "//tests:testdata/build_tar/world", ], python_version = "PY3", srcs_version = "PY3", diff --git a/tests/tar/build_tar_test.py b/tests/tar/build_tar_test.py index 1974a155..a5ffd573 100644 --- a/tests/tar/build_tar_test.py +++ b/tests/tar/build_tar_test.py @@ -28,8 +28,8 @@ def setUp(self): super(TarFileUnitTest, self).setUp() self.tempfile = os.path.join(os.environ["TEST_TMPDIR"], "test.tar") self.data_files = runfiles.Create() - test_file_name = "hello.txt" - self.directory = self.data_files.Rlocation("rules_pkg/tests/testdata/" + test_file_name) + test_file_name = "world" + self.directory = self.data_files.Rlocation("rules_pkg/tests/testdata/build_tar/" + test_file_name) # Remove the file name to get directory. if (self.directory.endswith(test_file_name)): self.directory = self.directory[:-len(test_file_name)] @@ -45,7 +45,7 @@ def test_add_tree(self): with build_tar.TarFile(self.tempfile, "/", "", "", None) as tar_file_obj: tar_file_obj.add_tree(self.directory + "/", "/") helper.assertTarFileContent(self, self.tempfile, [ - {"name": "./hello.txt", "data": "Hello, world!\n".encode("utf-8")}, + {"name": "./world", "data": "Hello, world!\n".encode("utf-8")}, ]) diff --git a/tests/testdata/build_tar/world b/tests/testdata/build_tar/world new file mode 100644 index 00000000..af5626b4 --- /dev/null +++ b/tests/testdata/build_tar/world @@ -0,0 +1 @@ +Hello, world!