add 'Runtime' struct to make test easier; add create test #447

liangchenye · 2017-08-24T14:02:46Z

Signed-off-by: Liang Chenye [email protected]

liangchenye · 2017-08-24T14:06:39Z

Hi @Mashimiao , this is my approach to test lifecycle.
We need more test cases to cover 'create' operation.
Mark as WIP now.

wking · 2017-08-24T17:17:38Z

validation/validation_test.go

-	runtime = os.Getenv("RUNTIME")
+	runtimeInEnv := os.Getenv("RUNTIME")
+	if runtimeInEnv != "" {
+		runtime = runtimeInEnv


This looks like a non-WIP improvement to me. Can we move it into its own PR to get it landed more quickly?

#448 is created.

wking · 2017-08-24T17:22:08Z

validation/validation_test.go

 }

-func runtimeValidate(runtime string, g *generate.Generator) error {
+func runtimeValidate(runtime string, g *generate.Generator, args []string) error {


This is not going to work, because when you test start, state, delete etc. you'll need to call create first (in most cases). runtimeValidate contains all the bundle setup and teardown code, so you need a way to call multiple runtime commands inside the test, not just a way to change which single runtime command you call. Pulling the bundle creation and teardown functionality out into separate functions seems like a more flexible approach, allowing callers to easily do whatever they want in between the two.

Your are right, move 'create/start/state/delete' to 'args' is not enough.
#391 @Mashimiao is the way to prepare the bundle.

liangchenye · 2017-08-25T12:51:53Z

It is similar with @Mashimiao 's #391.
I just add a 'Runtime' struct and add more test cases in 'create' test.

In current testing, runc does not follow the lifecycle spec...

liangchenye · 2017-08-26T09:54:18Z

Updated, PTAL @wking @Mashimiao @mrunalp @q384566678

liangchenye · 2017-08-30T03:44:28Z

change to [WIP] . the stretchr/testify pkg is not included.
I'll add it in #451

Signed-off-by: Liang Chenye <[email protected]>

liangchenye · 2017-08-30T08:06:00Z

Updated with #451 rfc code feature.
PTAL @q384566678 @Mashimiao

Mashimiao · 2017-08-30T08:51:56Z

LGTM

zhouhao3 · 2017-08-30T09:05:23Z

LGTM

wking · 2017-08-30T22:28:27Z

validation/validation_test.go

-	g.SetRootPath(".")
-	g.SetProcessArgs([]string{"/runtimetest"})
-	return &g
+	return r.Start()


This is broken, since you've deferred a delete call (via Clean) earlier in the function. That doesn't work reliably though, because delete MUST generate an error if the container is not stopped, and the fact that start completes doesn't mean you're stopped (you could also be running). In order for Clean() to work reliably, you either need to attempt to send a KILL to the container process with kill (although the spec is not clear on what signals are supported, you need something like #321 to see that KILL MUST be supported) or code that waits for the container process to exit on its own (more on this here, in #305, and in most of the other PRs that attempted to split run into create andstart, e.g. here and here).

Generated with: $ godep save ./... When I originally did this with v74 I needed to move the entries from Godeps/_workspace/src to vendor/ by hand, but now that I'm using godep v77 they're added to vendor/ automatically. I'm not sure why github.com/stretchr/testify/assert and descendants weren't added to Godeps.json back in 15577bd (add runtime struct; add create test, 2017-08-24, opencontainers#447), since that's when they landed in vendor/. The fact that they weren't there means it's hard to tell whether the changes my godep call made are moving the libraries forward or backward in time, but I expect they're moving forward because I just updated them with 'go get -u ...'. Signed-off-by: W. Trevor King <[email protected]>

Capture stdout and stderr from create invocations, because we don't want to pollute the TAP output with things like runc's: Incorrect Usage. ... (which is for some reason printed to stdout) or: runc: "create" requires exactly 1 argument(s) which is printed to stderr. Instead, show the captured stderr as a diagnostic, and hide the stdout completely. Unless stderr is empty, in which case show stdout in case it contains something useful. Most of these tests are broken because we aren't collecting the container exit code or post-start stdout. But the tests haven't been doing that since the create/start split in 15577bd (add runtime struct; add create test, 2017-08-24, opencontainers#447) anyway [1]. This commit just makes that more obvious. The patsubst and wildcard Makefile syntax is documented in [2]. The $(VALIDATION_TESTS) rule uses the static pattern rule syntax [3]. [1]: opencontainers#447 (comment) [2]: https://www.gnu.org/software/make/manual/html_node/Wildcard-Function.html [3]: https://www.gnu.org/software/make/manual/html_node/Static-Usage.html#Static-Usage Signed-off-by: W. Trevor King <[email protected]>

Capture stdout and stderr from create invocations, because we don't want to pollute the TAP output with things like runc's: Incorrect Usage. ... (which is for some reason printed to stdout) or: runc: "create" requires exactly 1 argument(s) which is printed to stderr. Instead, show the captured stderr as a diagnostic, and hide the stdout completely. Unless stderr is empty, in which case show stdout in case it contains something useful. Most of these tests are broken because we aren't collecting the container exit code or post-start stdout. But the tests haven't been doing that since the create/start split in 15577bd (add runtime struct; add create test, 2017-08-24, opencontainers#447) anyway [1]. This commit just makes that more obvious. The patsubst and wildcard Makefile syntax is documented in [2]. The $(VALIDATION_TESTS) rule uses the static pattern rule syntax [3]. And use 'console' instead of 'sh' in the README, because these are shell sessions, not scripts. See [4,5]. I don't have a working runc locally, so the mock results based on a dummy runtime script. [1]: opencontainers#447 (comment) [2]: https://www.gnu.org/software/make/manual/html_node/Wildcard-Function.html [3]: https://www.gnu.org/software/make/manual/html_node/Static-Usage.html#Static-Usage [4]: https://help.github.com/articles/creating-and-highlighting-code-blocks/#syntax-highlighting [5]: https://github.com/github/linguist/blob/v5.3.3/lib/linguist/languages.yml#L4249-L4260 Signed-off-by: W. Trevor King <[email protected]>

liangchenye force-pushed the unittest branch from 51249e3 to a7d1dc3 Compare August 24, 2017 14:04

wking reviewed Aug 24, 2017

View reviewed changes

liangchenye force-pushed the unittest branch from a7d1dc3 to ae802f3 Compare August 25, 2017 12:47

liangchenye force-pushed the unittest branch 3 times, most recently from 5a90e8a to 5d52992 Compare August 26, 2017 09:52

liangchenye changed the title ~~[WIP] add create test~~ add 'Runtime' struct to make test easier; add create test Aug 26, 2017

liangchenye changed the title ~~add 'Runtime' struct to make test easier; add create test~~ [WIP] add 'Runtime' struct to make test easier; add create test Aug 30, 2017

add runtime struct; add create test

15577bd

Signed-off-by: Liang Chenye <[email protected]>

liangchenye force-pushed the unittest branch from 5d52992 to 15577bd Compare August 30, 2017 08:02

liangchenye changed the title ~~[WIP] add 'Runtime' struct to make test easier; add create test~~ add 'Runtime' struct to make test easier; add create test Aug 30, 2017

zhouhao3 merged commit e285bae into opencontainers:master Aug 30, 2017

wking mentioned this pull request Aug 30, 2017

Split runtime validate #391

Closed

wking reviewed Aug 30, 2017

View reviewed changes

wking mentioned this pull request Nov 26, 2017

cmd/runtimetest/main: Use TAP diagnostics for errors #439

Merged

add 'Runtime' struct to make test easier; add create test #447

add 'Runtime' struct to make test easier; add create test #447

Uh oh!

Conversation

liangchenye commented Aug 24, 2017

Uh oh!

liangchenye commented Aug 24, 2017

Uh oh!

wking Aug 24, 2017

Choose a reason for hiding this comment

Uh oh!

liangchenye Aug 25, 2017

Choose a reason for hiding this comment

Uh oh!

wking Aug 24, 2017

Choose a reason for hiding this comment

Uh oh!

liangchenye Aug 25, 2017

Choose a reason for hiding this comment

Uh oh!

liangchenye commented Aug 25, 2017

Uh oh!

liangchenye commented Aug 26, 2017

Uh oh!

liangchenye commented Aug 30, 2017

Uh oh!

liangchenye commented Aug 30, 2017

Uh oh!

Mashimiao commented Aug 30, 2017 • edited by caniszczyk Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zhouhao3 commented Aug 30, 2017 • edited by caniszczyk Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wking Aug 30, 2017

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Mashimiao commented Aug 30, 2017 •

edited by caniszczyk

Loading

zhouhao3 commented Aug 30, 2017 •

edited by caniszczyk

Loading