Add unit tests for TarFile class (for #668)

[sqybi]

The unit tests mentioned in #668 .
This PR only covers unit tests for the case which #668 fixed, and a simple test for normalize_path interface. It's far from enough but a good start.

[sqybi]

I need Windows environment to reproduce the test failure on Windows platform. I will fix it tomorrow.

[aiuto]

I am unavailable for the next week or two, so I can't look deeply until March.
That aside, this seems too complex at first. All you needed for a test case was new test data that could exploit the old problem. Changing the test infrastructure at all seems far too complex.

[sqybi]

In this PR, I did:

• Move assertTarFileContent() function to helper.py which was in tar_write_test.py, because it could be used in both tar_write_test and my new unit test.
• Add two simple tests for build_tar.TarFile, which is test_normalize_path and test_add_tree. The latter is to exploit the problem in Fix a potential TypeException caused by None type #668.
• Add new test data for the unit test test_add_tree, and append the information of the new file to glob_for_texts_manifest.golden, which is necessary. The new test data is for the case there is a folder to add and can improve the test coverage for test_add_tree.

To make the test simpler, what I can do is:

• Remove the test for test_normalize_path.
• Remove the new test data.

I'm glad to make those changes later. However, the PR may still look complex after that.

[sqybi]

I have removed the test for normalize_path.

However, it seems when the test was running in Windows environment, all files under testdata were copied to runfiles.Create().Rlocation("rules_pkg/tests/testdata/") although I just declared data = [ "//tests:testdata/hello.txt" ] in the BUILD file. See error logs: https://buildkite.com/bazel/rules-pkg/builds/2248 .
I don't know how to fix that, so I still need a subdirectory and put my test files in this directory. The directory is named build_tar now.
Any ideas for removing the build_tar subdirectory would be appreciated.

[aiuto]

I'm OOO this week. Will look next week.

…

On Wed, Feb 22, 2023, 2:35 AM Qingyu Sui ***@***.***> wrote: I have removed the test for normalize_path. However, it seems when the test was running in Windows environment, all files under testdata were copied to runfiles.Create().Rlocation("rules_pkg/tests/testdata/") although I just declared data = [ "//tests:testdata/hello.txt" ] in the BUILD file. See error logs: https://buildkite.com/bazel/rules-pkg/builds/2248 . I don't know how to fix that, so I still need a subdirectory and test files in this directory. The directory is named build_tar now. Any ideas for removing the build_tar subdirectory would be appreciated. — Reply to this email directly, view it on GitHub <#675 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAXHHHFR5UU4GQGS2BUUNHLWYWXRRANCNFSM6AAAAAAU7H2V3U> . You are receiving this because your review was requested.Message ID: ***@***.***>

[aiuto]

I think this is not an effective test. It sets up the bad condition within the test harness instead of creating the problem situation with normal inputs such as pkg_files targets.
I am presuming you were motivated to #668 because you had a real situation where it occurred. Can you produce a scaled down version of that input, and the test is to simply built the tar file from that.

[sqybi]

@aiuto Unfortunately we could never reproduce the issue with pkg_files-like targets.
It's because, in build_tar.py, there is a file_attributes() function which always generates a non-empty mode. See this link.
In our case, we made some modifications in this rules_pkg repo instead of just calling the targets. We called add_tree() from our code without calling main() and add_manifest_entry() -- that's the only way to reproduce this issue, as I did in the test.
However, I think we still need to fix this issue. That's because we may call add_tree() elsewhere someday in the future, and we should never assume that piece of code can generate a non-empty mode as file_attributes() did. After all, the default value of mode is None in the declaration of add_tree().