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

make repo size test pass deterministically #4443

Merged
merged 1 commit into from
Dec 3, 2017
Merged

Conversation

Stebalien
Copy link
Member

@Stebalien Stebalien commented Nov 30, 2017

This test assumed that the repo could only grow in size when it can also shrink. This commit makes the test actually test for the specific bug it was meant to detect.

fixes #4408

@ghost ghost assigned Stebalien Nov 30, 2017
@ghost ghost added the status/in-progress In progress label Nov 30, 2017
@@ -22,7 +22,8 @@ test_expect_success "'ipfs repo stat' RepoSize is correct with sym link" '
export IPFS_PATH=".ipfs" &&
reposize_symlink=$(ipfs repo stat | grep RepoSize | awk '\''{ print $2 }'\'') &&
echo "reposize_symlink: $reposize_symlink; reposize_direct: $reposize_direct" &&
test $reposize_symlink -ge $reposize_direct
reposize_diff="$(( $reposize_symlink - $reposize_direct ))" &&
test "${reposize_diff#-}" -lt 2048 # less than 2k diff
'
Copy link
Contributor

Choose a reason for hiding this comment

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

Any particular reason for choosing "2048" as the threshold?

Copy link
Member Author

Choose a reason for hiding this comment

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

1k was too small in my testing.

Copy link
Contributor

Choose a reason for hiding this comment

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

I am thinking the threshold should be larger. If there was a symbolic link that was not followed then I image either 1) there will be an error or 2) the size will be 0 or something very close to it...

Copy link
Contributor

Choose a reason for hiding this comment

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

Prior to the PR to add support for symbolic links, the RepoSize for a symbolic link .ipfs path was non-zero but small. I don't remember exactly what it was but I think it was <100 bytes.

Copy link
Member Author

Choose a reason for hiding this comment

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

In that case, the correct fix would probably be to just check if the repo-size (as seen via a symlink) is over 1KiB. Sound good?

Copy link
Contributor

Choose a reason for hiding this comment

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

I would test @leerspace assumption first by undue his change and seeing what size is reported. But otherwise that sounds good.

Copy link
Member Author

Choose a reason for hiding this comment

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

Tested and fixed. I'm now testing to make sure that the repo size is greater than the size of the symlink itself.

Copy link
Contributor

Choose a reason for hiding this comment

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

That seams a little overly specific, but if that the size that was being reported before it LGTM.

Copy link
Member Author

Choose a reason for hiding this comment

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

I could just choose some arbitrary minimum but this is really what we're trying to test.

Copy link
Contributor

Choose a reason for hiding this comment

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

Either way is fine by me.

@Stebalien Stebalien changed the title add fuzz to repo size test make repo size test pass deterministically Dec 1, 2017
@whyrusleeping
Copy link
Member

sharness tests seem to be failing on travis

Before, we'd check to make sure the repo, when checked through a symlink, is at
least as large as the repo *before* we checked it through the symlink. However,
this assumes that the repo can't shrink.

Really, this test exists to ensure we measure the repo size itself instead of
the size of the symlink; this commit changes the test to reflect this.

This test fails when 54d7e03 is reverted.

fixes #4408

License: MIT
Signed-off-by: Steven Allen <[email protected]>
@Stebalien
Copy link
Member Author

You know... cross platform shell scripts are a real pain.

@whyrusleeping whyrusleeping merged commit 39f0f5f into master Dec 3, 2017
@whyrusleeping whyrusleeping deleted the fix/4408 branch December 3, 2017 17:05
@ghost ghost removed the status/in-progress In progress label Dec 3, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

RepoSize incorrect with symlink
4 participants