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

test: Stub in sharness framework #180

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

wking
Copy link
Contributor

@wking wking commented Aug 3, 2016

Spun off from this discussion. This currently ignores most of the generate options and doesn't touch validate at all, but I thought I'd float it early for some feedback before sinking too much time into it. If the current work looks solid, I'm happy to land it now and follow up with additional PRs to fill it out, or to keep this PR open until the coverage is more complete.

Details on the individual changes in the commit messages.

ocitools generate --template template | jq . >actual &&
cat <<-EOF >expected &&
{
\"ociVersion\": \"1.0.0-rc1\",
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't like the escaping that we do here :/

Copy link
Contributor Author

Choose a reason for hiding this comment

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

On Wed, Aug 03, 2016 at 04:06:28PM -0700, Mrunal Patel wrote:

@@ -0,0 +1,59 @@
+#!/bin/sh
+
+test_description='Test generate template'
+
+. ./sharness.sh
+
+test_expect_failure CAT,ECHO,HEAD,JQ 'Test ocitools generate --template with an empty template' "

  • echo '{}' >template &&
  • ocitools generate --template template | jq . >actual &&
  • cat <<-EOF >expected &&
  •   {
    
  •     \"ociVersion\": \"1.0.0-rc1\",
    

I don't like the escaping that we do here :/

There are only so many quotes we can use ;). Would you rather me wrap
the test in single quotes and use double quotes inside it? That makes
using variables in the test tricky, but we don't do any of that yet.

@liangchenye
Copy link
Member

The sharness framework looks cool, I need time to absorb it.

It is different test frame topic with #66, right? It is more like a unit test. If so, I think we go test is a suitable and more nature choice since generate/validate are all writing in go.

@wking
Copy link
Contributor Author

wking commented Aug 4, 2016

On Thu, Aug 04, 2016 at 05:34:29AM -0700, 梁辰晔 (Liang Chenye) wrote:

It is different test frame topic with
#66, right?

Yes. That is about “how do we validate a runtime?” and this is about
“how do we validate ocitools?”. I think they both benefit from an
off-the-shelf test harness, but you can use different harnesses for
each.

It is more like a unit test. If so, I think we go test is a
suitable and more nature choice since generate/validate are all
writing in go.

The goal of this PR is to test the ocitools command line as a black
box. I'd prefer ‘go test’ for testing the generation library API, but
that's not what I'm aiming at here.

@liangchenye
Copy link
Member

+1 for the black box test

@wking
Copy link
Contributor Author

wking commented Oct 12, 2016

Is this dead, or worth a rebase?

@Mashimiao
Copy link

Compared with importing sharness, how about importing a go based framework, like go-check?

@wking
Copy link
Contributor Author

wking commented Oct 13, 2016

On Wed, Oct 12, 2016 at 11:04:24PM -0700, Ma Shimiao wrote:

Compared with importing sharness, how about importing a go based
framework, like go-check?

A shell-based test harness lets us use a very convenient API for
executing subprocesses: shell syntax. go-check [1,2] looks like a
nice, generic test framework, but you'd have to use os/exec 3 to
execute a command, manage its standard streams, and collect its
exit status. With sharness, you can take care of all of that with a single
line like 4:

ocitools --help | head -n2 >actual &&

@Mashimiao
Copy link

Please contribute to master branch

@Mashimiao Mashimiao closed this Jul 19, 2017
@wking
Copy link
Contributor Author

wking commented Jul 19, 2017

Please contribute to master branch.

I can rebase onto master and change the target branch without filing a new PR and fragmenting discussion. Can you re-open this PR so I can do that?

@Mashimiao Mashimiao reopened this Jul 19, 2017
@wking wking changed the base branch from v1.0.0.rc1 to master July 19, 2017 15:34
@wking
Copy link
Contributor Author

wking commented Jul 19, 2017 via email

@@ -15,3 +15,4 @@ script:
- git-validation -run DCO,short-subject -v -range ${TRAVIS_COMMIT_RANGE}
- make
- make test
- PATH="${PATH}:${PWD}" make -C test

Choose a reason for hiding this comment

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

I think this will be failed. As from test/README.md, we need do 'git submodule update --init' first. But you didn't do such preparation in travis.yml

Copy link
Contributor Author

Choose a reason for hiding this comment

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

... we need do 'git submodule update --init' first.

Travis does that for us.

Choose a reason for hiding this comment

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

OK, got it.

test/README.md Outdated
# oci-runtime-tool integration tests

This project uses the [Sharness][] test harness, installed as a [Git
submodule]submodule]. To setup the test installation after a clone,
Copy link
Contributor Author

@wking wking Aug 2, 2017

Choose a reason for hiding this comment

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

I'm missing a [ here. Will update later tonight.

@Mashimiao
Copy link

I'm OK with this patchset, but generate related tests are all failed. Would you mind update these tests based on current generate output?

@wking
Copy link
Contributor Author

wking commented Aug 2, 2017

.. generate related tests are all failed...

They look like they passed to me. Can you link to what you'd like fixed?

@Mashimiao
Copy link

Mashimiao commented Aug 2, 2017

My local test result:

not ok 1 - Test oci-runtime-tool generate writing to stdout
#	
#		oci-runtime-tool generate | head -n2 >actual &&
#		cat <<EOF >expected &&
#	{
#		"ociVersion": "1.0.0-rc6",
#	EOF
#		test_cmp expected actual
#	
not ok 2 - Test oci-runtime-tool generate --output
#	
#		oci-runtime-tool generate --output config.json &&
#		head -n2 config.json >actual &&
#		cat <<EOF >expected &&
#	{
#		"ociVersion": "1.0.0-rc6",
#	EOF
#		test_cmp expected actual
#	
# failed 2 among 2 test(s)
1..2```

@wking
Copy link
Contributor Author

wking commented Aug 2, 2017

Ah, I probably need to rebase around #431.

It's GPLv2+ [1], so using a submodule keeps the separation from our
Apache 2.0 source clear.  And it's an isolated enough dependency that
I prefer to keep the Git histories separate.

Generated with:

  $ mkdir test
  $ git submodule add git://github.com/mlafeldt/sharness.git test/sharness
  $ (cd test/sharness && git checkout v1.0.0)
  $ git add test/sharness
  $ cd test
  $ ln -s sharness/aggregate-results.sh
  $ ln -s sharness/sharness.sh
  $ ln -s sharness/test/Makefile
  $ emacs README.md
  $ git add README.md Makefile sharness.sh aggregate-results.sh

[1]: https://github.com/mlafeldt/sharness/tree/v1.0.0#license

Signed-off-by: W. Trevor King <wking@tremily.us>
Signed-off-by: W. Trevor King <wking@tremily.us>
Signed-off-by: W. Trevor King <wking@tremily.us>
Using jq to format the output so we don't have to worry about
oci-runtime-tool's default tab indents or lack of trailing newlines,
neither of which play nicely with <<-EOF here documents.

Signed-off-by: W. Trevor King <wking@tremily.us>
Signed-off-by: W. Trevor King <wking@tremily.us>
@wking
Copy link
Contributor Author

wking commented Aug 2, 2017 via email

@Mashimiao
Copy link

Mashimiao commented Aug 2, 2017

LGTM

Approved with PullApprove

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