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

Jump to bottom

sharness: replace POSIX prereq with STD_ERR_MSG #2392

Merged
merged 1 commit into from
Feb 29, 2016
+13 −3
Merged

sharness: replace POSIX prereq with STD_ERR_MSG #2392

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
4 changes: 3 additions & 1 deletion test/sharness/lib/test-lib.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,9 @@ test "$TEST_EXPENSIVE" = 1 && test_set_prereq EXPENSIVE
test "$TEST_NO_DOCKER" != 1 && type docker && test_set_prereq DOCKER

TEST_OS=$(uname -s | tr [a-z] [A-Z])
test expr "$TEST_OS" : "CYGWIN_NT" >/dev/null && test_set_prereq POSIX

# Set a prereq as error messages are often different on Windows/Cygwin
expr "$TEST_OS" : "CYGWIN_NT" >/dev/null || test_set_prereq STD_ERR_MSG
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we have a little comment here explaining what this does and why it's necessary?

Copy link
Contributor

Choose a reason for hiding this comment

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

Nitpick 2: can we use a more specific name than STD_ERR_MSG? It seems like this is being used to capture a very specific output difference.

Copy link
Member

Choose a reason for hiding this comment

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

"not windows error message" basically. I think STD_ERR_MSG is fine, could probably just add a comment about it

Copy link
Contributor

Choose a reason for hiding this comment

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

Can we invert this to be more informative? Set CYGWIN_ERROR_MSGS if we're on Windows+Cygwin?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@noffle we could do that, but if we skip some tests we are more likely to skip tests on Windows/Cygwin or other non standard platforms, so a STD_ERR_MSG prereq is more likely to be useful than a CYGWIN_ERROR_MSGS prereq.

Copy link
Contributor

Choose a reason for hiding this comment

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

"standard platform" is sort of a vague statement. If Cygwin is explicitly the special case, I'd much rather read a test that was concrete about why it was branching:

if test_have_prereq CYGWIN; then
    init_err_msg="Error: mkdir $IPFS_PATH: The system cannot find the path specified."
 else
    init_err_msg="Error: failed to take lock at $IPFS_PATH: permission denied"
 fi

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@noffle the idea is that outside Linux/MacOS the error messages can be very different.
Also testing error messages is not very useful and makes tests harder to port as we can see with Cygwin/Windows. Hence Git for example is not testing error messages much if at all.

If people start to port IPFS to exotic platforms, especially platforms where there is no tool like Travis or Appveyor that we can use online, it could be very difficult to add tests that check error messages, because they will fail on those exotic platform as people who have no access to these exotic platforms cannot know what the error message will be on those platforms.

So I think we should not try to be very picky about this, and having a STD_ERR_MSG prereq that we can easily stick to the tests that check error messages is the right tool.

I think I was even too zealous when I added this part:

+# Under Windows/Cygwin the error message is different,
+# so we use the STD_ERR_MSG prereq.
+if test_have_prereq STD_ERR_MSG; then
+   init_err_msg="Error: failed to take lock at $IPFS_PATH: permission denied"
+else
+   init_err_msg="Error: mkdir $IPFS_PATH: The system cannot find the path specified."
+fi

I should just have sticked STD_ERR_MSG to the test and be done with it. Now it's not a problem as long as people don't port to exotic platforms, so we can wait for that to happen and take another look at this at that time.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@noffle about "standard"/"STD", I am ok with finding another name for that. Previously I used "POSIX", so perhaps "POSIX_ERR_MSG" is better, otherwise I am open to suggestions.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't want to look for a FreeBSD system to test on right now, as I don't think many FreeBSD users have asked for help with running IPFS. And should we also find out for OpenBSD, NetBSD and so on?

This is exactly the point I'm trying to make: let's use LINUX_OR_OSX because we don't know at this moment whether any other platforms give compatible error messages or not. Let's name things and act based on what behaviours we know, not what we suspect.

Copy link
Contributor

Choose a reason for hiding this comment

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

(Sorry @chriscool not trying to bikeshed your work, I've just seen this pattern play out before where vague or inaccurate names for globals or config bits are chosen and people lose hours in the future.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So what about a name like "LINUX_LIKE_ERRORS"?

I think this is both more descriptive than "STD_ERR_MSG" and more future proof than "LINUX_OR_OSX".

Also I like that it is specific to errors because when differences are not specific to errors we should not use a prereq but instead do something like what we do in do_umount() in "test-lib.sh":

do_umount() {
    if [ "$(uname -s)" = "Linux" ]; then
    fusermount -u "$1"
    else
    umount "$1"
    fi
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Which platforms are "Linux-like" though? If we don't know and can't quantify it (are BSDs?), then we probably shouldn't claim that it's so.

Also I like that it is specific to errors because when differences are not specific to errors we should not use a prereq but instead do something like what we do in do_umount()

Agreed. Since these are just platforms checks ("are we NOT cygwin?"), let's just use if statements like your example above. This prereq seems like an unnecessary semantic abstraction if all we're doing is wanting to know if we're on cygwin or not.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Which platforms are "Linux-like" though? If we don't know and can't quantify it (are BSDs?), then we probably shouldn't claim that it's so.

As I see it, we are not claiming anything by calling a prereq "LINUX_LIKE_ERRORS". What do you think we are claiming?

If we later learn that another OS doesn't pass some tests that have this prereq, then we can just avoid setting this prereq when we are on this OS.

What I want is not just to know if we are on Cygwin or not. What I want is a way to easily avoid error message checking tests on platforms/OSes that give different error messages.

For example if the prereq is named NOT_CYGWIN and we later find that another OS, let say HAIKU also gives different error messages I don't want to have to add another NOT_HAIKU prereq to all the error checking tests.


if test "$TEST_VERBOSE" = 1; then
echo '# TEST_VERBOSE='"$TEST_VERBOSE"
Expand Down
12 changes: 10 additions & 2 deletions test/sharness/t0020-init.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,8 +19,16 @@ test_expect_success "ipfs init fails" '
test_must_fail ipfs init 2> init_fail_out
'

test_expect_success POSIX "ipfs init output looks good" '
echo "Error: failed to take lock at $IPFS_PATH: permission denied" > init_fail_exp &&
# Under Windows/Cygwin the error message is different,
# so we use the STD_ERR_MSG prereq.
if test_have_prereq STD_ERR_MSG; then
init_err_msg="Error: failed to take lock at $IPFS_PATH: permission denied"
else
init_err_msg="Error: mkdir $IPFS_PATH: The system cannot find the path specified."
fi

test_expect_success "ipfs init output looks good" '
echo "$init_err_msg" >init_fail_exp &&
test_cmp init_fail_exp init_fail_out
'

Expand Down