-
Notifications
You must be signed in to change notification settings - Fork 143
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
Validation tests #336
Validation tests #336
Conversation
@Mashimiao @liangchenye @wking PTAL |
Signed-off-by: Mrunal Patel <mrunalp@gmail.com>
Signed-off-by: Mrunal Patel <mrunalp@gmail.com>
Signed-off-by: Mrunal Patel <mrunalp@gmail.com>
e6c0451
to
ff2332e
Compare
On Mon, Mar 06, 2017 at 02:25:00PM -0800, Mrunal Patel wrote:
This is an alternative to #326 that uses go tests.
I think the concern about Go's builtin test framework was that you
couldn't install an executable to run the tests [1]. You'd have to
run runtime-validation tests from a source checkout. But I personally
have no problem with requiring validation tests to be run from a
source checkout [2], so whatever you find easiest to maintain works
for me.
[1]: #47 (comment)
[2]: #61 (comment)
|
We have merged #326, I think we can continue working on it |
@hqhq @liangchenye @Mashimiao I think we should prefer this approach as it will be easier to write tests using go's testing support rather than building our own. |
Using Go's builtin test framework seems easier. But in my opinion, runtime-tools should be a tool set, runtime validation is one tool of it. Validating runtime by |
@liangchenye @Mashimiao I think that rather than reworking this to use oci-runtime-tool runtime-validate, the approach in this PR is better as can directly access generate object and adjust it's properties. If we switch to command invocation then we will have to duplicate all options for generate to support test cases. A person executing the test case shouldn't care how this is implemented. |
On Fri, Mar 17, 2017 at 12:09:46PM -0700, Mrunal Patel wrote:
I think that rather than reworking this to use oci-runtime-tool
runtime-validate, the approach in this PR is better as can directly
access generate object and adjust it's properties.
You can directly access the generate object with both approaches,
can't you? Compare [1,2].
If we switch to command invocation then we will have to duplicate
all options for generate to support test cases.
I think the difference between the two cases is that with this PR you
can use ‘go test …’'s builtin harness, while with the approach in
#326 you'd need to use an installable harness (like ginkgo). The
decision probably comes down to “using the builtin harness is more
convenient for runtime-tools maintainers” vs. “using an installed
command is more convenient for runtime validators”, but I'm not clear
enough on either of those to know which has more weight.
[1]: https://github.com/opencontainers/runtime-tools/blob/ce55f9b813244c5054b3cccee882f594cacb9ca8/cmd/oci-runtime-tool/runtime_validate.go#L64
[2]: https://github.com/opencontainers/runtime-tools/pull/336/files#diff-b5e8ff579203d652582cf33ed2b39ad9R78
|
@wking I was talking of an approach where we modify 336 to invoke oci-runtime-tool runtime-validate test case. |
On Fri, Mar 17, 2017 at 01:37:54PM -0700, Mrunal Patel wrote:
@wking I was talking of an approach where we modify 336 to invoke
oci-runtime-tool runtime-validate test case.
Is that a thing? I think we want:
* Something that the tester runs (e.g. ‘oci-runtime-tool
runtime-validate’ or ‘go test ./...’).
* A test harness (e.g. ginkgo or Go's builtin harness)
* A test suite which that harness can drive.
* A binary or two to be used in some of the suite's tests
(e.g. busybox, runtimetest, …).
If you're using a Go test harness, I think it would live inside
runtime-validate. The test harness would not be calling
runtime-validate as part of a single test (I agree that that would
make accessing the generated config too awkward [1]).
However, I'm not aware of any PRs to use ginko as a harness. #9 had a
locally-implemented harness and this PR is using Go's builtin harness.
If there's a possibility that someone will PR an installable harness
into master soon (as @Mashimiao and @liangchenye seem to want [2,3]),
then that would be great. If that PR is unlikely to materialize soon,
I think we should land this one so we can move on. We can always
translate tests between harnesses if we change our mind about
harnesses later.
[1]: #336 (comment)
[2]: #336 (comment)
[3]: #336 (comment)
|
After discussion with @mrunalp and @Mashimiao , I'm fine with this solution. We can focus on test targets and don't need to distract by the framework at present |
1 similar comment
This reverts commit da62d5b. The installable approach to runtime validation has been obsoleted by the 'go test ...' approach from 24ca87f (Add tests for runtime validation, 2017-03-06, opencontainers#336). This commit drops the installable tests so we don't have to maintain both. Signed-off-by: W. Trevor King <wking@tremily.us>
This is an alternative to #326 that uses go tests.
The tests can be invoked as
RUNTIME=runc make localvalidation