add 'Runtime' struct to make test easier; add create test #447
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Hi @Mashimiao , this is my approach to test lifecycle. |
wking
reviewed
Aug 24, 2017
validation/validation_test.go
Outdated
| runtime = os.Getenv("RUNTIME") | ||
| runtimeInEnv := os.Getenv("RUNTIME") | ||
| if runtimeInEnv != "" { | ||
| runtime = runtimeInEnv |
There was a problem hiding this comment.
This looks like a non-WIP improvement to me. Can we move it into its own PR to get it landed more quickly?
validation/validation_test.go
Outdated
| } | ||
|
|
||
| func runtimeValidate(runtime string, g *generate.Generator) error { | ||
| func runtimeValidate(runtime string, g *generate.Generator, args []string) error { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Your are right, move 'create/start/state/delete' to 'args' is not enough.
#391 @Mashimiao is the way to prepare the bundle.
|
It is similar with @Mashimiao 's #391. In current testing, |
liangchenye
force-pushed
the
unittest
branch
3 times, most recently
from
5a90e8a
to
5d52992
Compare
liangchenye
changed the title
|
Updated, PTAL @wking @Mashimiao @mrunalp @q384566678 |
liangchenye
changed the title
|
change to [WIP] . the |
Signed-off-by: Liang Chenye <liangchenye@huawei.com>
|
Updated with #451 rfc code feature. |
liangchenye
changed the title
|
LGTM |
1 similar comment
|
LGTM |
Closed
wking
reviewed
Aug 30, 2017
| g.SetRootPath(".") | ||
| g.SetProcessArgs([]string{"/runtimetest"}) | ||
| return &g | ||
| return r.Start() |
There was a problem hiding this comment.
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).
wking
added a commit
to wking/ocitools-v2
that referenced
this pull request
Sep 9, 2017
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 <wking@tremily.us>
wking
added a commit
to wking/ocitools-v2
that referenced
this pull request
Sep 11, 2017
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 <wking@tremily.us>
wking
added a commit
to wking/ocitools-v2
that referenced
this pull request
Sep 11, 2017
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 <wking@tremily.us>
wking
added a commit
to wking/ocitools-v2
that referenced
this pull request
Sep 11, 2017
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 <wking@tremily.us>
wking
added a commit
to wking/ocitools-v2
that referenced
this pull request
Nov 26, 2017
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 <wking@tremily.us>
wking
added a commit
to wking/ocitools-v2
that referenced
this pull request
Nov 28, 2017
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 <wking@tremily.us>
wking
added a commit
to wking/ocitools-v2
that referenced
this pull request
Nov 28, 2017
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 <wking@tremily.us>
wking
added a commit
to wking/ocitools-v2
that referenced
this pull request
Nov 30, 2017
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 <wking@tremily.us>
wking
added a commit
to wking/ocitools-v2
that referenced
this pull request
Nov 30, 2017
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 <wking@tremily.us>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Signed-off-by: Liang Chenye liangchenye@huawei.com