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 t0130-swarm.sh #948

Closed
wants to merge 1 commit into from
Closed

Add t0130-swarm.sh #948

wants to merge 1 commit into from

Conversation

kkoroviev
Copy link
Contributor

Online tests for ipfs swarm addrs.

IFS='' read -r line || [[ -n $line ]] || return
grep $'^\t/[^/]*/[^/]*/[^/]*/[^/]*$' <<<"$line" >/dev/null || return
done
done
Copy link
Member

Choose a reason for hiding this comment

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

While this may work, it's confusing and hard to edit. It may be better to do what other things do, and compare the actual output to an expected output.

Addresses may be tricky, because interfaces differ, but can check for 127.0.0.1.

Another approach still may be the check the json output.

by the way, we can have dedicated programs in other languages that check these outputs, so doesn't have to be in sh

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah this uses a lot of bashisms like <<<"$line" while others tests should work with a lot of POSIX compatible shells.

Also cat <<<"$line" looks a bit overengineered compared to just echo "$line" :-)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@chriscool

Also cat <<<"$line" looks a bit overengineered compared to just echo "$line" :-)

I do agree :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jbenet

What language should I use? If everyone's gonna use their favorite language to write test helpers, the code base will inevitably become an unrecognizable mess.

Copy link
Contributor

Choose a reason for hiding this comment

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

@jbenet @kkoroviev my opinion about languages is that we should not require too many of them to test go-ipfs. And about shells I think it is better to try to be compatible with many POSIX compatible shells, so that ipfs scripts should not be too difficult to port to embeded or unix like systems.

Copy link
Member

Choose a reason for hiding this comment

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

I agree wholeheartedly with @chriscool

We should stick to POSIX for shells.

There are some tests where it may become easier to test things with tools written in Go. Go is good enough, multi platform, really easy to work with (tooling), and already required for the project. So perhaps we can:

  • default to do everything with sh
  • for convoluted things where a small go tool can really help, try go. but still compile it down to a unixy tool to be put into a sharness test.
  • for now, no other languages-- as it might introduce another dependency, which would be painful.

@kkoroviev
Copy link
Contributor Author

@chriscool

All the tests start with #!/bin/sh it is strange that this one requires bash.

That's because of the aforementioned unnecessary bashisms.

@jbenet
Copy link
Member

jbenet commented Mar 28, 2015

ping

@whyrusleeping whyrusleeping added the kind/test Testing work label Mar 28, 2015
@kkoroviev
Copy link
Contributor Author

@jbenet

Gonna work on it when we solve all my issues here: #962.

@whyrusleeping
Copy link
Member

closing due to inactivity, please reopen as necessary

note: all pull requests older than three weeks may be closed in an effort to keep our open pull requests more focused.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/test Testing work
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants