Skip to content

Conversation

@forsyth2
Copy link
Collaborator

@forsyth2 forsyth2 commented Nov 24, 2025

Summary

Objectives:

Issue resolution:

Select one: This pull request is...

  • a bug fix: increment the patch version
  • a small improvement: increment the minor version
  • a new feature: increment the minor version
  • an incompatible (non-backwards compatible) API change: increment the major version

Small Change

  • To merge, I will use "Squash and merge". That is, this change should be a single commit.
  • Logic: I have visually inspected the entire pull request myself.
  • Pre-commit checks: All the pre-commits checks have passed.

@forsyth2 forsyth2 self-assigned this Nov 24, 2025
@forsyth2 forsyth2 added the Testing Files in `tests` modified label Nov 24, 2025
check_log_has "zstash" ls_${case_name}_src2_output.log
ls ${src_dir}/${case_name}/zstash_demo/zstash 2>&1 | tee ls_${case_name}_src3_output.log
check_log_has "index.db" ls_${case_name}_src3_output.log
check_log_does_not_have "tar" ls_${case_name}_src3_output.log # tar is deleted at src
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Passes for blocking, fails for non-blocking, confirming #374.

@forsyth2 forsyth2 mentioned this pull request Nov 24, 2025
16 tasks
@forsyth2
Copy link
Collaborator Author

Using the code of #405, all 4 tests pass. Using the code of main the following failures occur:

  • Test 2 (test_globus_tar_deletion ${path_to_repo} LCRC_IMPROV_DTN_ENDPOINT ${chrysalis_dst_dir} "non-blocking" "non-keep") fails with Not-expected grep '000000.tar' was found in ls_non-blocking_non-keep_src3_output.log. Test failed., which reproduces the error of [Bug]: tar files are not deleted after successful globus transfer #374. That is, tar files were not being deleted after successful transfer when --non-blocking (but not --keep) was set.
  • Test 4 (test_globus_tar_deletion ${path_to_repo} LCRC_IMPROV_DTN_ENDPOINT ${chrysalis_dst_dir} "non-blocking" "keep") fails with Expected grep 'non-blocking_keep' not found in ls_non-blocking_keep_dst_output.log. Test failed. That's an interesting failure. I can check:
> ls /home/ac.forsyth2/zstash_tests/test_globus_tar_deletion_test_main_try2/
blocking_keep  blocking_non-keep  non-blocking_keep
> ls /home/ac.forsyth2/zstash_tests/test_globus_tar_deletion_test_main_try2/non-blocking_keep/
000000.tar  index.db

# Yet, we only get:
> cat /home/ac.forsyth2/ez/zstash/tests/utils/globus_tar_deletion/non-blocking_keep/ls_non-blocking_keep_dst_output.log
blocking_keep
blocking_non-keep

@forsyth2
Copy link
Collaborator Author

Remaining TODO:

@forsyth2 forsyth2 mentioned this pull request Dec 2, 2025
7 tasks
Copy link
Collaborator Author

@forsyth2 forsyth2 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@chengzhuzhang This is just a test addition, but let me know if you would like to review before I merge it.

The results are:

==========================================
TEST RESULTS
==========================================
✓ blocking_non-keep PASSED
✗ non-blocking_non-keep FAILED
✓ blocking_keep PASSED
✓ non-blocking_keep PASSED
==========================================
TEST SUMMARY
==========================================
Total tests: 4
Passed: 3
Failed: 1
==========================================

This is exactly as expected, given #374. I decided to make this test addition a distinct PR from the fix #405 because I wanted to make sure it showed this failure when testing on main. Alternatively, this test could simply be merged as part of that PR.

@chengzhuzhang
Copy link
Collaborator

@forsyth2 I I think the change looks okay. my preference would be having this type of specific testing change together with the actual code change to reduce the number of PR and review cycle.

@forsyth2
Copy link
Collaborator Author

forsyth2 commented Dec 3, 2025

my preference would be having this type of specific testing change together with the actual code change to reduce the number of PR and review cycle.

Ok, I can include it as part of #405 then. I mainly just wanted to make sure it did in fact fail on main, which is of course not quite possible if it's included on a branch with fixes to main.

@chengzhuzhang
Copy link
Collaborator

Ok, I can include it as part of #405 then. I mainly just wanted to make sure it did in fact fail on main, which is of course not quite possible if it's included on a branch with fixes to main.

I think you can just merge this pull request as is. For future testing PRs, we can consider to bundle with the PR with actual code change.

@forsyth2
Copy link
Collaborator Author

forsyth2 commented Dec 3, 2025

Sounds good, thanks, merging.

@forsyth2 forsyth2 merged commit 01ffe34 into main Dec 3, 2025
5 checks passed
@forsyth2 forsyth2 deleted the test-tar-deletion branch December 3, 2025 23:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Testing Files in `tests` modified

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants