Open sqybi opened 1 year ago
I need Windows environment to reproduce the test failure on Windows platform. I will fix it tomorrow.
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.
In this PR, I did:
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.build_tar.TarFile
, which is test_normalize_path
and test_add_tree
. The latter is to exploit the problem in #668.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:
test_normalize_path
.I'm glad to make those changes later. However, the PR may still look complex after that.
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.
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 https://github.com/bazelbuild/rules_pkg/pull/675#issuecomment-1439507826, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAXHHHFR5UU4GQGS2BUUNHLWYWXRRANCNFSM6AAAAAAU7H2V3U . You are receiving this because your review was requested.Message ID: @.***>
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.
@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()
.
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.