sharness: replace POSIX prereq with STD_ERR_MSG #2392

chriscool · 2016-02-22T19:42:18Z

This fixes some previous mistakes I made and replaces PR #2368

chriscool · 2016-02-22T20:01:27Z

Again "No output has been received in the last 10m0s, ..." errors with Travis on OSX.

chriscool · 2016-02-22T20:24:23Z

Actually there is also this error in the go tests:

?       github.com/ipfs/go-ipfs/exchange    [no test files]
SIGQUIT: quit
PC=0x460611 m=0

goroutine 5 [syscall, 9 minutes]:
runtime.notetsleepg(0xc8a500, 0xffffffffffffffff, 0x1)
    /usr/local/go/src/runtime/lock_futex.go:202 +0x4e fp=0xc820029f40 sp=0xc820029f18
runtime.signal_recv(0x0)
    /usr/local/go/src/runtime/sigqueue.go:111 +0x132 fp=0xc820029f78 sp=0xc820029f40
os/signal.loop()
    /usr/local/go/src/os/signal/signal_unix.go:22 +0x18 fp=0xc820029fc0 sp=0xc820029f78
runtime.goexit()
    /usr/local/go/src/runtime/asm_amd64.s:1721 +0x1 fp=0xc820029fc8 sp=0xc820029fc0
created by os/signal.init.1
    /usr/local/go/src/os/signal/signal_unix.go:28 +0x37

goroutine 1 [chan receive, 9 minutes]:
testing.RunTests(0xa18f88, 0xc4e340, 0x9, 0x9, 0x40f701)
    /usr/local/go/src/testing/testing.go:562 +0x8ad
testing.(*M).Run(0xc82b7c3f08, 0x911c60)
    /usr/local/go/src/testing/testing.go:494 +0x70
main.main()
    github.com/ipfs/go-ipfs/exchange/bitswap/_test/_testmain.go:70 +0x116

hackergrrl · 2016-02-22T20:24:29Z

test/sharness/lib/test-lib.sh

@@ -45,7 +45,7 @@ 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
+expr "$TEST_OS" : "CYGWIN_NT" >/dev/null || test_set_prereq STD_ERR_MSG


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

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.

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

License: MIT Signed-off-by: Christian Couder <chriscool@tuxfamily.org>

hackergrrl · 2016-02-22T21:20:59Z

test/sharness/lib/test-lib.sh

-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


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

@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.

"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

@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.

@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.

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.

(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.)

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 }

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.

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.

chriscool · 2016-02-22T21:31:48Z

Circle CI fails with:

expecting success: 
  yes | ipfs daemon --init >stdin_daemon_out 2>stdin_daemon_err &
  pollEndpoint -ep=/version -v -tout=1s -tries=10 >stdin_poll_apiout 2>stdin_poll_apierr &&
  test_kill_repeat_10_sec $! ||
  test_fsh cat stdin_daemon_out || test_fsh cat stdin_daemon_err || test_fsh cat stdin_poll_apiout || test_fsh cat stdin_poll_apierr

> cat stdin_daemon_out
Initializing daemon...

> cat stdin_daemon_err

> cat stdin_poll_apiout

> cat stdin_poll_apierr
21:20:29.988 DEBUG pollEndpoi: starting at %s, tries: %d, timeout: %s, url: %s2016-02-22 21:20:29.988450776 +0000 UTC 10 1s {http  <nil> 127.0.0.1:5001 /version   } main.go:57
21:20:29.989 DEBUG pollEndpoi: get failed: Get http://127.0.0.1:5001/version: dial tcp 127.0.0.1:5001: getsockopt: connection refused main.go:66
21:20:30.989 DEBUG pollEndpoi: get failed: Get http://127.0.0.1:5001/version: dial tcp 127.0.0.1:5001: getsockopt: connection refused main.go:66
21:20:31.990 DEBUG pollEndpoi: get failed: Get http://127.0.0.1:5001/version: dial tcp 127.0.0.1:5001: getsockopt: connection refused main.go:66
21:20:32.990 DEBUG pollEndpoi: get failed: Get http://127.0.0.1:5001/version: dial tcp 127.0.0.1:5001: getsockopt: connection refused main.go:66
21:20:33.991 DEBUG pollEndpoi: get failed: Get http://127.0.0.1:5001/version: dial tcp 127.0.0.1:5001: getsockopt: connection refused main.go:66
21:20:34.991 DEBUG pollEndpoi: get failed: Get http://127.0.0.1:5001/version: dial tcp 127.0.0.1:5001: getsockopt: connection refused main.go:66
21:20:35.991 DEBUG pollEndpoi: get failed: Get http://127.0.0.1:5001/version: dial tcp 127.0.0.1:5001: getsockopt: connection refused main.go:66
21:20:36.992 DEBUG pollEndpoi: get failed: Get http://127.0.0.1:5001/version: dial tcp 127.0.0.1:5001: getsockopt: connection refused main.go:66
21:20:37.992 DEBUG pollEndpoi: get failed: Get http://127.0.0.1:5001/version: dial tcp 127.0.0.1:5001: getsockopt: connection refused main.go:66
21:20:38.993 DEBUG pollEndpoi: get failed: Get http://127.0.0.1:5001/version: dial tcp 127.0.0.1:5001: getsockopt: connection refused main.go:66
21:20:39.993 ERROR pollEndpoi: failed. main.go:71

not ok 21 - 'ipfs daemon' should be able to run with a pipe attached to stdin (issue #861)

whyrusleeping · 2016-02-24T02:35:01Z

@chriscool filed an issue for that failure here: #2400

It appears to be unrelated though, is this PR ready to go?

chriscool · 2016-02-24T09:27:03Z

@whyrusleeping please wait a bit more until hopefully the name issue is resolved.

hackergrrl · 2016-02-24T18:09:05Z

The naming discussion is non-blocking: please feel free to go ahead and merge if this is blocking other work or making CI redder than it need be. 👍

chriscool · 2016-02-25T21:20:55Z

Yeah @whyrusleeping please merge this as is.

sharness: replace POSIX prereq with STD_ERR_MSG

hackergrrl reviewed Feb 22, 2016
View reviewed changes

sharness: replace POSIX prereq with STD_ERR_MSG

00e0340

License: MIT Signed-off-by: Christian Couder <chriscool@tuxfamily.org>

chriscool force-pushed the std-err-msg-prereq branch from 3758bb5 to 00e0340 Compare February 22, 2016 21:14

hackergrrl reviewed Feb 22, 2016
View reviewed changes

chriscool added the RFM label Feb 25, 2016

chriscool mentioned this pull request Feb 29, 2016

RFM: go-ipfs: sharness: replace POSIX prereq with STD_ERR_MSG whyrusleeping/todo#2

Closed

whyrusleeping added a commit that referenced this pull request Feb 29, 2016

Merge pull request #2392 from ipfs/std-err-msg-prereq

922ad3c

sharness: replace POSIX prereq with STD_ERR_MSG

whyrusleeping merged commit 922ad3c into master Feb 29, 2016

whyrusleeping deleted the std-err-msg-prereq branch February 29, 2016 23:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

sharness: replace POSIX prereq with STD_ERR_MSG #2392

sharness: replace POSIX prereq with STD_ERR_MSG #2392

chriscool commented Feb 22, 2016

chriscool commented Feb 22, 2016

chriscool commented Feb 22, 2016

hackergrrl Feb 22, 2016

hackergrrl Feb 22, 2016

whyrusleeping Feb 22, 2016

hackergrrl Feb 22, 2016

chriscool Feb 22, 2016

hackergrrl Feb 23, 2016

chriscool Feb 23, 2016

chriscool Feb 23, 2016

hackergrrl Feb 23, 2016

hackergrrl Feb 23, 2016

chriscool Feb 24, 2016

hackergrrl Feb 24, 2016

chriscool Feb 24, 2016

chriscool commented Feb 22, 2016

whyrusleeping commented Feb 24, 2016

chriscool commented Feb 24, 2016

hackergrrl commented Feb 24, 2016

chriscool commented Feb 25, 2016

sharness: replace POSIX prereq with STD_ERR_MSG #2392

sharness: replace POSIX prereq with STD_ERR_MSG #2392

Conversation

chriscool commented Feb 22, 2016

chriscool commented Feb 22, 2016

chriscool commented Feb 22, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chriscool commented Feb 22, 2016

whyrusleeping commented Feb 24, 2016

chriscool commented Feb 24, 2016

hackergrrl commented Feb 24, 2016

chriscool commented Feb 25, 2016