Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(22)

Issue 6852065: golxc: add first container tests (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
7 years ago by TheMue
Modified:
6 years, 10 months ago
Reviewers:
mp+134956
Visibility:
Public.

Description

golxc: add first container tests The package now has the first functions for the testing of the container. Two external scripts help to perform the tests as a root. https://code.launchpad.net/~themue/golxc/001-added-sudo-and-first-test/+merge/134956 (do not edit description out of merge proposal)

Patch Set 1 #

Patch Set 2 : golxc: add first container tests #

Patch Set 3 : golxc: add first container tests #

Total comments: 44

Patch Set 4 : golxc: add first container tests #

Total comments: 6

Patch Set 5 : golxc: add first container tests #

Total comments: 6

Patch Set 6 : golxc: add first container tests #

Total comments: 41

Patch Set 7 : golxc: add first container tests #

Total comments: 2

Patch Set 8 : golxc: add first container tests #

Total comments: 2

Patch Set 9 : golxc: add first container tests #

Total comments: 2

Patch Set 10 : golxc: add first container tests #

Unified diffs Side-by-side diffs Delta from patch set Stats (+406 lines, -190 lines) Patch
A [revision details] View 1 2 3 4 5 6 7 8 9 1 chunk +2 lines, -0 lines 0 comments Download
A export_test.go View 1 chunk +6 lines, -0 lines 0 comments Download
M golxc.go View 1 2 3 4 5 6 7 8 9 8 chunks +143 lines, -190 lines 0 comments Download
A golxc_test.go View 1 2 3 4 5 6 7 8 1 chunk +247 lines, -0 lines 0 comments Download
A golxc_test.sh View 1 2 3 4 5 6 7 8 1 chunk +8 lines, -0 lines 0 comments Download

Messages

Total messages: 18
TheMue
Please take a look.
7 years ago (2012-11-19 16:14:59 UTC) #1
TheMue
Please take a look.
7 years ago (2012-11-20 07:45:07 UTC) #2
TheMue
Please take a look.
7 years ago (2012-11-20 09:47:37 UTC) #3
rog
lots of comments but nothing major. i'm not familiar with lxc, so all superficial, i'm ...
7 years ago (2012-11-20 15:20:11 UTC) #4
TheMue
Please take a look. https://codereview.appspot.com/6852065/diff/6001/golxc.go File golxc.go (right): https://codereview.appspot.com/6852065/diff/6001/golxc.go#newcode9 golxc.go:9: // Frank Mueller <frank.mueller@canonical.com> On ...
7 years ago (2012-11-20 16:34:04 UTC) #5
rog
LGTM apart from the error messages. i think we don't want to discard the important ...
7 years ago (2012-11-20 16:49:53 UTC) #6
TheMue
Please take a look. https://codereview.appspot.com/6852065/diff/11001/golxc.go File golxc.go (right): https://codereview.appspot.com/6852065/diff/11001/golxc.go#newcode35 golxc.go:35: // LogLevel represents a containers ...
7 years ago (2012-11-21 08:36:56 UTC) #7
fwereade
Not sure about the tests -- it looks like there's plenty of opportunity from pollution ...
7 years ago (2012-11-21 17:32:53 UTC) #8
TheMue
Please take a look. https://codereview.appspot.com/6852065/diff/7007/golxc.go File golxc.go (right): https://codereview.appspot.com/6852065/diff/7007/golxc.go#newcode217 golxc.go:217: _, _, err := run("lxc-wait", ...
7 years ago (2012-11-22 12:53:23 UTC) #9
fwereade
A few more comments... https://codereview.appspot.com/6852065/diff/6002/golxc.go File golxc.go (right): https://codereview.appspot.com/6852065/diff/6002/golxc.go#newcode180 golxc.go:180: // Unfreeze thaws all a ...
7 years ago (2012-11-22 14:35:57 UTC) #10
TheMue
Please take a look. https://codereview.appspot.com/6852065/diff/6002/golxc.go File golxc.go (right): https://codereview.appspot.com/6852065/diff/6002/golxc.go#newcode180 golxc.go:180: // Unfreeze thaws all a ...
7 years ago (2012-11-22 15:20:50 UTC) #11
fwereade
LGTM!
7 years ago (2012-11-22 15:36:46 UTC) #12
rog
looks good to me apart from (still) the error messages. a few minor suggestions in ...
7 years ago (2012-11-22 15:38:31 UTC) #13
TheMue
Please take a look. https://codereview.appspot.com/6852065/diff/9003/golxc_test.go File golxc_test.go (right): https://codereview.appspot.com/6852065/diff/9003/golxc_test.go#newcode12 golxc_test.go:12: type LXCSuite struct{} On 2012/11/22 ...
7 years ago (2012-11-22 16:55:02 UTC) #14
rog
much better, but still not *quite* there. i'd like to see at least one test ...
7 years ago (2012-11-23 10:58:19 UTC) #15
TheMue
Please take a look. https://codereview.appspot.com/6852065/diff/5003/golxc.go File golxc.go (right): https://codereview.appspot.com/6852065/diff/5003/golxc.go#newcode29 golxc.go:29: if strings.HasPrefix(e.StdErr, e.Name) { On ...
7 years ago (2012-11-23 13:46:48 UTC) #16
rog
LGTM with one suggestion below. i'm worried i might have led you up the garden ...
7 years ago (2012-11-23 14:02:55 UTC) #17
TheMue
7 years ago (2012-11-23 15:59:19 UTC) #18
*** Submitted:

golxc: add first container tests

The package now has the first functions for the
testing of the container. Two external scripts
help to perform the tests as a root.

R=rog, fwereade
CC=
https://codereview.appspot.com/6852065

https://codereview.appspot.com/6852065/diff/18/golxc.go
File golxc.go (right):

https://codereview.appspot.com/6852065/diff/18/golxc.go#newcode326
golxc.go:326: if len(output) == 0 {
On 2012/11/23 14:02:55, rog wrote:
> i think this might be better as something like:
> 
>     e := &Error{name, err, nil}
>     for _, l := range strings.Split(string(output), "\n") {
>         if l != "" {
>             e.Output = append(e.Output, l)
>         }
>     }
>     return e
> 
> i think that's equivalent, except it also removes all blank lines, which could
> be considered a good thing.
> 
> if you wanted, this would also be a good place to strip the command prefix
from
> all the lines.

Done.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b