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

Use Sharness as our shell test framework, version 2 #204

Merged
merged 5 commits into from
Oct 25, 2014

Conversation

chriscool
Copy link
Contributor

This builds on the previous pull-request #201 (Use Sharness as our shell test framework).
This adds a Makefile and a script to aggregate the results from all the test scripts.

The first two commits are the same as in the previous pull-request #201, so no need to merge the previous if you merge this one.

all: clean $(T) aggregate

clean:
rm -r test-results
Copy link
Member

Choose a reason for hiding this comment

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

this (and also just make) will fail if there is no test-results directory. maybe:

    -rm -r test-results

@jbenet
Copy link
Member

jbenet commented Oct 25, 2014

@chriscool thanks! Works great, as expected I get:

jbenet @ lorien : ~/go/src/github.com/jbenet/go-ipfs/test * chriscool-sharness2 % make
rm -r test-results
*** t0010-basic-commands.sh ***
ok 1 - current dir is writable
ok 2 - ipfs version succeeds
ok 3 - ipfs version output looks good
ok 4 - ipfs help succeeds
ok 5 - ipfs help output looks good
# passed all 5 test(s)
1..5
./test-aggregate-results.sh
fixed   0
success 5
failed  0
broken  0
total   5

Three points:

  1. make will fail the first time (as flagged above).
  2. Currently, running these tests requires an extra installation step. Go tools in general vendor everything (see Godeps directory). So let's do that with sharness too. https://github.com/mlafeldt/sharness#per-project-installation says we can just add sharness.sh, right?
  3. The files include a copyright notice, but no license. Currently all code committed to the project bears the same license: MIT* -- I know it's common for every file to bear copyright notices but it gets messy with multiple edits. (the world still needs to catch up to accepting git commit histories instead of bare files!) Would you consider adding a note below the copyright line like so (or removing it if you don't care either way):
# Copyright (c) 2014 Christian Couder
# MIT Licensed; see the LICENSE file in this repository.

(* I've been pressed to switch from MIT to Apache 2 to protect us and ipfs users with the patent clause, and I'm still considering that. If that happens, everyone will receive a request to sign-off on the re-license.)

@jbenet
Copy link
Member

jbenet commented Oct 25, 2014

Once this is in, the next step for #148 will be to list out a number of tests to perform.

Our test framework is based on Sharness.
So the first thing to do is to source it.
This checks a little bit the installation and some
basic commands.

You can run it like that:

$ cd test
$ ./t0010-basic-commands.sh
 ok 1 - current dir is writable
 ok 2 - ipfs version succeeds
 ok 3 - ipfs version output looks good
 ok 4 - ipfs help succeeds
 ok 5 - ipfs help output looks good
 # passed all 5 test(s)
 1..5
This way we can easily reuse the checks in
test-sharness-config.sh.
This script aggregates test results using Sharness.
You can use it like this to launch all the
test scripts in order:

$ cd test
$ make
 rm -r test-results
 *** t0010-basic-commands.sh ***
 ok 1 - current dir is writable
 ok 2 - ipfs version succeeds
 ok 3 - ipfs version output looks good
 ok 4 - ipfs help succeeds
 ok 5 - ipfs help output looks good
 # passed all 5 test(s)
 1..5
 ./test-aggregate-results.sh
 fixed   0
 success 5
 failed  0
 broken  0
 total   5

Or you can just run one test like this:

$ make t0010-basic-commands.sh
 *** t0010-basic-commands.sh ***
 ok 1 - current dir is writable
 ok 2 - ipfs version succeeds
 ok 3 - ipfs version output looks good
 ok 4 - ipfs help succeeds
 ok 5 - ipfs help output looks good
 # passed all 5 test(s)
 1..5
@chriscool
Copy link
Contributor Author

Ok with points 1 and 3. I will send a new PR soon.

About point 2, yeah I can just add sharness.sh (and aggregate-results.sh), but as it is GPLV2+, I thought that it was safer to install it separately. Now if you think it's safe, I am ok with adding it.

By the way about git commits, I was at a talk at LinuxCon NA last August and Bradley Kuhn said that it would be safer if the developers added a trailer like this:

License: MIT

at the end of each commit, even if they already add a Signed-off-by trailer like:

Signed-off-by: Christian Couder chriscool@tuxfamily.org

(Which is mandatory when you contribute to the Linux kernel or to Git see "Sign your work" in: https://github.com/git/git/blob/master/Documentation/SubmittingPatches)

According to Bradley Kuhn, people refused to add a trailer "License: GPLv2" because it felt like a burden. But note that "git interpret-trailers" (that I developed) is now in Git's master branch and it makes it easier to automatically add whatever trailer you want at the end of each commit message.

So my opinion is that the world could soon accept git commit histories as the real thing :-)

@jbenet
Copy link
Member

jbenet commented Oct 25, 2014

@chriscool About point 2, yeah I can just add sharness.sh (and aggregate-results.sh), but as it is GPLV2+, I thought that it was safer to install it separately. Now if you think it's safe, I am ok with adding it.

Ah yikes, yeah good catch. Let's not add it then.

added a trailer...Signed-off-by trailer

Yeah, I think this would be nice. I worry about adding the extra overhead on the contributors, but it's important to ensure proper OSS hygiene. Docker asks for Signed-off-by's as well: https://github.com/docker/docker/blob/master/CONTRIBUTING.md#sign-your-work Let's discuss more on this over here: #207

So my opinion is that the world could soon accept git commit histories as the real thing :-)

Ah, hopefully!

@jbenet
Copy link
Member

jbenet commented Oct 25, 2014

@chriscool and, if you just push to your sharness2 branch, it should be updated here in this same PR.

@chriscool
Copy link
Contributor Author

Thanks for merging this!
Now do you want to list out a number of tests to perform here or in issue #148?.

Note that it would be easier to add tests if ipfs can be run as a regular user (not just root) and if its config file location and mount directories can be configured.

I tried:

$ ipfs -c="some_directory/.go-ipfs" init

but it didn't work. I will have a look.

@jbenet
Copy link
Member

jbenet commented Oct 25, 2014

I'll list out tests later tonight.

 Ipfs is meant to run as user. The config flag is --config, though it may be broken until commands refactor lands. The ENV var should work though:

    IPFS_DIR=foo/bar/.go-ipfs


Sent from Mailbox

On Sat, Oct 25, 2014 at 8:53 AM, Christian Couder
notifications@github.com wrote:

Thanks for merging this!
Now do you want to list out a number of tests to perform here or in issue #148?.
Note that it would be easier to add tests if ipfs can be run as a regular user (not just root) and if its config file location and mount directories can be configured.
I tried:
$ ipfs -c="some_directory/.go-ipfs" init

but it didn't work. I will have a look.

Reply to this email directly or view it on GitHub:
#204 (comment)

@chriscool chriscool mentioned this pull request Oct 26, 2014
@aschmahmann aschmahmann mentioned this pull request Aug 23, 2021
62 tasks
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.

2 participants