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

add sharness test for pubsub #3432

Merged
merged 2 commits into from
Dec 5, 2016
Merged

Conversation

keks
Copy link
Contributor

@keks keks commented Nov 28, 2016

finally!

@whyrusleeping
Copy link
Member

Looks like it fails on sharness (at least on teamcity): http://ci.ipfs.team:8111/viewLog.html?buildId=8663&buildTypeId=GoIpfs_CiTests&tab=buildLog

cc @chriscool for review, this is a bit of strange shell hackery

# ipfs pubsub sub
test_expect_success 'pubsub' '
echo "testOK" > expected &&
mkfifo done &&
Copy link
Contributor

Choose a reason for hiding this comment

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

It's a bit confusing to call a fifo "done" as it is a keyword in shell.

while read line; do
echo $line > actual &&
echo > done
exit
Copy link
Contributor

Choose a reason for hiding this comment

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

It's a bit strange to have this "exit" here.
It means that the while loop will in fact never loop, so perhaps it could be an if read line; then ... fi instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah that is a bit hackish I agree. For some reason this loop even needs two runs, I didn't get why and I was glad it worked.

echo > done
exit
done
) &
Copy link
Contributor

Choose a reason for hiding this comment

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

It should be explained why we need to fork a process like this, especially as there is a "sleep 1" below.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah I'll add a comment

test_expect_success "start up nodes" '
iptb start
'
fi
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't is possible to do something less specific by doing something like:

num_nodes="$1"
shift
other_args="$@"

...

if test -n "$other_args"; then
    iptb start --args $other_args
else
    iptb start
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.

I tried iptb start $@ but now I see why that didn't work - its executed in a completely different context. I use your solution now.


ipfspid=$!

sleep 1
Copy link
Contributor

Choose a reason for hiding this comment

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

Even if the two above commands cannot fail it is a good practice to anyway chain them with the commands below by using "&&" after them.

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'll put && after the sleep. Where else do you mean?

ipfsi 1 pubsub pub testTopic "testOK" &> pubErr1 &&

cat done &&
test_cmp actual expected
Copy link
Contributor

Choose a reason for hiding this comment

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

test_cmp expected actual is more standard.

@chriscool
Copy link
Contributor

Yeah, it LGTM now, thanks!

@keks keks force-pushed the feat/pubsub-sharness branch 3 times, most recently from 421b68a to 2444e85 Compare December 1, 2016 16:57
@keks
Copy link
Contributor Author

keks commented Dec 2, 2016

@chriscool The tests pass for me locally but fail on Travis and Teamcity. Do you have an idea what the problem could be? Do they pass on your machine?

@whyrusleeping
Copy link
Member

@keks the problem is with your previous commit. Under the ndpayload encoding, youre outputting an empty line when the 'null' hello message arrives, where instead you should be outputting nothing (since the goal there is to ignore that first message)

@whyrusleeping
Copy link
Member

This line here: https://github.com/ipfs/go-ipfs/blob/master/core/commands/pubsub.go#L133

Should just return an empty string reader instead of a newline

@keks
Copy link
Contributor Author

keks commented Dec 3, 2016

Of course! Thank you. "The code is right, it must be the test's fault"...

I will move the checks to getPsMarshaler later today because now we have the same code in all three Marshalers.

License: MIT
Signed-off-by: Jan Winkelmann <[email protected]>
License: MIT
Signed-off-by: Jan Winkelmann <[email protected]>
@whyrusleeping
Copy link
Member

@keks Thanks a bunch! This is great to have :)

@whyrusleeping whyrusleeping merged commit 0c413de into ipfs:master Dec 5, 2016
@whyrusleeping whyrusleeping removed the status/in-progress In progress label Dec 5, 2016
@ghost ghost mentioned this pull request Dec 23, 2016
@keks keks deleted the feat/pubsub-sharness branch November 22, 2017 18:41
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.

4 participants