sharness/t0060-daemon: don't reuse expect/actual names

[cryptix]

If the names are reused, you might stare at the wrong output from another command, wondering what is going on.

[jbenet]

how was this passing? ...

[jbenet]

Oh it's disabled (test_expect_failure)

[jbenet]

@cryptix any idea why it's failing? have we fixed it?

[cryptix]

Not sure what you mean, It passes now.

I wanted to see if it does so in the test runners before I change the expectation.

[jbenet]

the test is disabled:

test_expect_failure ...

[jbenet]

ah it's fixed now nvm.

[chriscool]

I am not sure doing that this kind of changes is worth it.
Git has been using "test_cmp expected actual" or "test_cmp expect actual" a lot and people don't complain about it:

$ git grep test_cmp -- 't*.sh' | wc -l
3895
$ git grep 'test_cmp expect actual'  -- 't*.sh' | wc -l
1343
$ git grep 'test_cmp expected actual'  -- 't*.sh' | wc -l
567

[chriscool]

Also it might give a false sense of security while making test scripts more difficult to review.

For example if someone uses "expected_msg" at the top of a test script then you would want to make sure that "expected_msg" is used nowhere else in the same test script, which is difficult to maintain given that people often copy-paste tests.

[cryptix]

@chriscool in general i'd agree. The problem here was, that test_expect_success passed because it tested the wrong assumption. Later tests then overwrote the previous output files, making it impossible to trace the original one.

[cryptix]

I also don't like to make sure the names all add up but other than a nifty helper that creates these file names for me, I don't see how to deal with this other than stopping test execution after the first error, which might not be what we want for all tests (edit: which also isn't satisfactory in the light of false positives)?

[cryptix]

I think, this is what drove us in circles in #1277 - On dev box, there might be another daemon running (on test env there might be an unkilled daemon still running). The effect is that pollEndpoint passes by dialing another daemon.

[whyrusleeping]

yeah, this issue is why iptb polls the 'ipfs id' endpoint and compares the returned ID against the expected one.

[cryptix]

Hmm, yea... I could add that mode to pollEndpoint as well.

[jbenet]

we could dump out the node id on the /version endpoints

[jbenet]

Also it might give a false sense of security while making test scripts more difficult to review.

agreed

For example if someone uses "expected_msg" at the top of a test script then you would want to make sure that "expected_msg" is used nowhere else in the same test script, which is difficult to maintain given that people often copy-paste tests.

I'm not sure what a good solution. i think reusing the names can be annoying to debug. but also a partial solution isn't safe either.

dont think it's a big deal either way

[chriscool]

I'm not sure what a good solution. i think reusing the names can be annoying to debug. but also a partial solution isn't safe either.

Yeah, there is no easy way to be safe, we just have to accept that and be careful.

It's easier to be careful when you can do fast pattern matching which is easier if the names are very often the same. For example if I see test_cmp actual expected I can easily see that it should probably be test_cmp expected actual because I am used a lot to seing the later and not the former. But if I see test_cmp foo bar, it's more difficult to quickly see if there might be a problem or not.

By the way, I created the following PR https://github.com/mlafeldt/sharness/pull/20 to add test_pause() from Git which might help debugging.