Add test skip utils#1587
Conversation
|
This is kind of a PoC RFC. By providing a framework for skipping tests, we can make the skips clearer and provide more powerful abilities. @devimc - you can use this to skip a test on RHEL say by doing something like: var tc TestConstraint
func init() {
tc := NewTestConstraint(false)
}
func TestFoo(t *testing.T) {
if tc.NotValid(NeedDistroNotEquals("rhel")) {
t.Skipf("skipping test: %+v", tc)
}
// Test code...
} |
|
/test |
e71fd72 to
a4d7493
Compare
|
/retest |
|
18.04 initrd CI failed in spectacular Java fashion: Let's see if a rebuild resolves the spew... |
|
q35 CI is not happy |
|
Whilst we wait, I'll add some unit tests for the code next week... |
|
@devimc, @grahamwhaley - if you look at #1604, you'll see that the single failure is unrelated to this PR, so I think we can merge this one..? |
pkg/katatestutils/constraints.go
Outdated
|
|
||
| osRelease = "/etc/os-release" | ||
|
|
||
| // Clear Linux has a different path (for stateless support) |
There was a problem hiding this comment.
nit - The use of /usr/lib/os-release is part of the os-release spec, so may not only be Clear specific :-)
https://www.freedesktop.org/software/systemd/man/os-release.html
| @@ -0,0 +1,355 @@ | |||
| // Copyright (c) 2019 Intel Corporation | |||
There was a problem hiding this comment.
Should there be some accompanying documentation with these additions - detailing what/how/where you can apply constraints to the test files?
|
I'm going to add some tests, a doc and kernel version handling for #1604, so let's mark as dnm for now... |
a4d7493 to
634df67
Compare
|
Branch updated but not yet complete:
@klynnrif and @grahamwhaley - ptal at the new doc on this PR ( |
klynnrif
left a comment
There was a problem hiding this comment.
Completed a grammar/structure/flow review on pkg/katatestutils/README.md - a few suggested rewrites. Thanks!
pkg/katatestutils/README.md
Outdated
|
|
||
| ## Test Constraints | ||
|
|
||
| This package provides helper functions to allow tests to be skipped based on a |
There was a problem hiding this comment.
Lines 14-15 suggested rewrite (unless I've changed the meaning - previous sentence is a bit ambiguous):
This package provides helper functions that accept user-specified constraints that allow you to skip tests.
pkg/katatestutils/README.md
Outdated
| ### Usage | ||
|
|
||
| Create a `TestConstraint` object using the `NewTestConstraint()` constructor. | ||
| This takes a single boolean parameter which specifies if debug output should |
There was a problem hiding this comment.
Lines 20-21 suggested rewrite:
This takes a single Boolean parameter that specifies if debug output is generated.
pkg/katatestutils/README.md
Outdated
| This takes a single boolean parameter which specifies if debug output should | ||
| be generated. | ||
|
|
||
| In each test that needs to be skipped, call the `NotValid()` method on the |
There was a problem hiding this comment.
Lines 23-24: I'm not certain what the end of this sentence is trying relay - "passing one more constraint" or "passing on more constraints". I've removed the duplicate "more" but based on the intended meaning we might want to rewrite the end of the sentence as well.
In each test that is skipped, call the NotValid() method on the TestConstraint object, passing one more constraints that you want to be valid.
pkg/katatestutils/README.md
Outdated
|
|
||
| In each test that needs to be skipped, call the `NotValid()` method on the | ||
| `TestConstraint` object, passing one more more constraints that you want to be | ||
| valid. The `NotValid()` function will return `true` if any of the specified |
There was a problem hiding this comment.
Lines 25-27 suggested rewrite:
The NotValid() function returns true if any of the specified constraints are not available. This allows for a more natural way to code an arbitrarily complex test skip as shown in the following example:
pkg/katatestutils/README.md
Outdated
| constraints are not available. This allows for a natural way of coding an | ||
| arbitrarily complex test skip as shown below. | ||
|
|
||
| The main object can be created in the `init()` function to make it available to |
There was a problem hiding this comment.
Suggested rewrite:
The main object is created in the init() function to make it available for all tests:
|
@jodh-intel @lifupan This is similar to the rhel issue that we saw. See this: |
@amshinde @jodh-intel Yes, the root cause is the absent of "nsfs" in kernel, maybe we can check is the "nsfs" feature supported on the host instead of checking the kernel version before we run the specific testcase. |
634df67 to
773a47d
Compare
|
@klynnrif et al - branch has now been updated! /retest |
773a47d to
c5dd83b
Compare
|
/retest |
c5dd83b to
76fa53f
Compare
|
/retest |
|
All CI's are green apart from the known q35 issue (#1604). Please review y'all! |
76fa53f to
9053172
Compare
|
/retest |
9053172 to
8c46767
Compare
|
/retest |
|
Restarted one of the travis jobs which had failed due to the new linter timing out: /cc @ganeshmaharaj |
|
Ubuntu nemu failed with: sles 12: opensuse 15: And centos with the usual #1604 🌰. Restarted the failing ones (apart from centos). |
grahamwhaley
left a comment
There was a problem hiding this comment.
One nit, one question.
Will lgtm, to ease merging...
pkg/katatestutils/README.md
Outdated
|
|
||
| #### Displaying the TestConstraint | ||
|
|
||
| Note that you could add the TestConstraint` object to the `Skip()` call as it |
There was a problem hiding this comment.
back tick code block messed up in this line?
pkg/katatestutils/README.md
Outdated
|
|
||
| ### Full details | ||
|
|
||
| The public API is shown in [`constraints_api.go`](constraints_api.go). |
There was a problem hiding this comment.
any chance we can get that as a godoc, so we can see the go api in the same style one does for other go packages?
There was a problem hiding this comment.
Updated to point at the godoc site.
8c46767 to
669d646
Compare
Enhance the `katatestutils` package to provide the ability to skip tests based on either user or distro the tests are running on. Fixes kata-containers#1586. Signed-off-by: James O. D. Hunt <james.o.hunt@intel.com>
Updated the test code to use the new test constraints feature. Signed-off-by: James O. D. Hunt <james.o.hunt@intel.com>
669d646 to
23f7cfa
Compare
|
It would be great to avoid re-running all those CI's for a doc change, but I'll leave it up to others to decide that since it's my PR ;) |
|
meh... /retest |
| @@ -0,0 +1,38 @@ | |||
| # Gopkg.toml example | |||
There was a problem hiding this comment.
woot! , is this correct? I thought that there should only be one vendor directory
There was a problem hiding this comment.
It does seem to be required, yes:
INFO: Running 'go test' as current user on package 'github.com/kata-containers/runtime/cli' with flags '-v -race -timeout 30s -outputdir "/tmp/tmp.RnuwUOlXss"'
# github.com/kata-containers/runtime/cli
package github.com/kata-containers/runtime/cli (test)
imports github.com/kata-containers/runtime/pkg/katatestutils
imports github.com/blang/semver: cannot find package "github.com/blang/semver" in any of:
/home/james/go/src/github.com/kata-containers/runtime/vendor/github.com/blang/semver (vendor tree)
/usr/local/go/src/github.com/blang/semver (from $GOROOT)
/home/james/go/src/github.com/blang/semver (from $GOPATH)
FAIL github.com/kata-containers/runtime/cli [setup failed]... but:
$ dep ensure -add github.com/blang/semver
Fetching sources...
Failed to add the dependencies:
✗ github.com/blang/semver is already imported or required, so -add is only valid with a constraint
adding dependencies failed
$ grep -c semver Gopkg.*
Gopkg.lock:0
Gopkg.toml:0
$
We do have other packages with their own vendoring, for example:
|
It would be good to get this landed before the sands shift once more... 😄 |
|
Thanks for the updates. You have the acks, and we know the q35 is an anomaly right now... I think we can merge... |
Enhance the
katatestutilspackage to provide the ability to skip tests based on either user or distro the tests are running on.Fixes #1586.
Signed-off-by: James O. D. Hunt james.o.hunt@intel.com