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.

Sign up for GitHub

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

t0030-mount.sh: 2 known breakages vanished #945

Closed
wants to merge 4 commits into from
Closed

t0030-mount.sh: 2 known breakages vanished #945

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
ff7a333
t0030-mount.sh: 2 known breakages vanished
kkoroviev Mar 19, 2015
da93e15
t0030-mount.sh: Rewrite two tests into one
kkoroviev Mar 23, 2015
23a9ca7
t0030-mount.sh: Split 1 test into 3
kkoroviev Mar 23, 2015
0cd09f4
t0030-mount.sh: Merge 2 tests into 1
kkoroviev Mar 31, 2015
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 19 additions & 0 deletions test/sharness/lib/test-lib.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -276,3 +276,22 @@ test_curl_resp_http_code() {
cat curl_output
return 1
}

test_must_be_empty() {
if test -s "$1"
then
echo "'$1' is not empty, it contains:"
cat "$1"
return 1
fi
}

test_should_contain() {
test "$#" = 2 || error "bug in the test script: not 2 parameters to test_should_contain"
Copy link
Member

Choose a reason for hiding this comment

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

maybe "test_should_contain: requires 2 parameters"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I decided to mimic original errors: examples are here and here.

Copy link
Member

Choose a reason for hiding this comment

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

ah, okay. that's fine then. good enough for junio, good enough for me.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Though your variant is definitely better, uniformity is even more better.

if ! grep -q "$1" "$2"
then
echo "'$2' does not contain '$1', it contains:"
cat "$2"
return 1
fi
}
11 changes: 5 additions & 6 deletions test/sharness/t0030-mount.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,14 +20,13 @@ test_launch_ipfs_daemon

# run this mount failure before mounting properly.

test_expect_failure "'ipfs mount' fails when no mount dir (issue #341)" '
test_must_fail ipfs mount -f=not_ipfs -n=not_ipns >actual
test_expect_success "'ipfs mount' fails when there is no mount dir" '
test_must_fail eval "ipfs mount -f=not_ipfs -n=not_ipns >output 2>output.err"
'

test_expect_failure "'ipfs mount' looks good when it fails (issue #341)" '
! grep "IPFS mounted at: $(pwd)/ipfs" actual >/dev/null &&
! grep "IPNS mounted at: $(pwd)/ipns" actual >/dev/null ||
test_fsh cat actual
test_expect_success "'ipfs mount' output looks good" '
test_must_be_empty output &&
test_should_contain "not_ipns\|not_ipfs" output.err
'
Copy link
Member

Choose a reason for hiding this comment

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

why is this test case removed? it should remain. it may even be fixed once we vendor the new fuse.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The set of all possible outputs that don't contain "IPFS mounted at:" and "IPNS mounted at:" is too broad. What if someone changes our output message to "IPFS mounted at" (without the ":") and then someone introduces a bug that causes ipfs mount to print our output message? The test will pass. The test will pass under almost any circumstances. And it haven't helped us the last time.

The stdout output should contain nothing. The stderr output should contain not_ipns or not_ipfs. I test only for those conditions, and my test is good enough to handle almost every possible situation.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think the idea to have two tests is because the first test just checks that the exist code is what we expect and the second one tests that the output is what we expect.

Here the first one "'ipfs mount' fails when no mount dir" would just check that the exit code is not zero. And the second one should be renamed "'ipfs mount' output looks good when it fails" to make clear that it's about the output.

This is a bit different than in git where the exit code and the output are checked in the same test, but I think it makes it easier to debug.

Copy link
Member

Choose a reason for hiding this comment

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

The set of all possible outputs that don't contain "IPFS mounted at:" and "IPNS mounted at:" is too broad. What if someone changes our output message to "IPFS mounted at" (without the ":") and then someone introduces a bug that causes ipfs mount to print our output message? The test will pass. The test will pass under almost any circumstances. And it haven't helped us the last time.

It is critical that we test our output formats as exactly as we can, so that if we accidentally change the output format we notice. People will build scripts on top of our outputs and they will only be able to do so as long as we do not change the outputs unless absolutely necessary. This should be a very explicit decision, and test cases help us make sure we do this carefully.


# now mount properly, and keep going
Expand Down