Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix testing/hashtest.py (#78) #290

Closed
wants to merge 1 commit into from
Closed

Conversation

zjw
Copy link
Contributor

@zjw zjw commented Mar 8, 2020

This fixes and reenables the failing hash test identified in #78. Currently,
rdiff-backup only stores a hash with the first file in a series of linked
files. The test has now been modfied to recognize that fact.

Issue #78 also pointed out certain warnings that are generated about missing
SHA1 digests. It's believed that those warnings have already been fixed by
pull request #239.

This fixes and reenables the failing hash test identified in rdiff-backup#78.  Currently,
rdiff-backup only stores a hash with the first file in a series of linked
files.  The test has now been modfied to recognize that fact.

Issue rdiff-backup#78 also pointed out certain warnings that are generated about missing
SHA1 digests.  It's believed that those warnings have already been fixed by
pull request rdiff-backup#239.
@ericzolf
Copy link
Member

ericzolf commented Mar 8, 2020

Sorry, I can't accept this patch: the issue is not that the test expects a hash, the issue is IMHO that the functions RORPath.has_sha1/get_sha1/set_sha1 don't handle properly hashes for hard-linked files. The interface of the object should hide the fact that the hash is only stored once, and return transparently the hash of the first stored hard-link.

Given the fact that the test doesn't fail (because it's ignored), I don't see either the need for a temporary fix.

I only keep the PR opened if you have arguments I oversaw, but else I'd close it.

@zjw
Copy link
Contributor Author

zjw commented Mar 9, 2020

I don't dispute anything you said. However, the self-described purpose of this test_session(self) is:

Run actual sessions and make sure proper hashes recorded

The way the test accomplishes its purpose is that it opens the actual mirror_metadata file in order to inspect its contents and to assure that the hashes have been recorded as expected. The expected result is that there is a proper hash stored with the first file in a series of hardlinked files, but that no hash is stored with subsequent files in the series. So this test is focused on the contents of the mirror_metadata file. I haven't changed it focus -- It's just that the test was looking for the wrong results inside the file, and I have now corrected it.

Your issue is that RORPath.has_sha1/get_sha1/set_sha1 don't properly handle hashes for hard-linked files, and that these functions should transparently return the hash of the first stored hard-link. In other words, those functions should "just work" (regardless of any possible hardlinking). I agree; that would be nice. You also state that those functions should hide the fact that the hash is only stored once in the mirror_metadata file. Again, I agree. But this test case is not about any of that. This test is simply confirming that the mirror_metadata file was written correctly, and that is what the test does. You could make changes to RORPath.has_sha1/get_sha1/set_sha1 so that they "just work", and this test case would still function properly as-is, because it is looking at the underlying mirror_metadata file. So I do not see this as any sort of temporary fix.

This pull request also fixes a pesky issue with a binary string which should be committed, regardless -- thus possibly saving somebody the trouble of tracking down why that part of the code isn't working like it should.

@ericzolf
Copy link
Member

The test isn't looking at the underlying structure, it's using xxx_sha1 which has the wrong interface IMHO and would fail if we'd change this. Where I can accept workarounds when they make the software work, I don't see the point where it's just making an additional, already skipped, test to work. Hence I'm closing the PR.

@ericzolf ericzolf closed this Mar 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants