{"action":"created","issue":{"url":"https://api.github.com/repos/mistifyio/lochness/issues/121","repository_url":"https://api.github.com/repos/mistifyio/lochness","labels_url":"https://api.github.com/repos/mistifyio/lochness/issues/121/labels{/name}","comments_url":"https://api.github.com/repos/mistifyio/lochness/issues/121/comments","events_url":"https://api.github.com/repos/mistifyio/lochness/issues/121/events","html_url":"https://github.com/mistifyio/lochness/pull/121","id":125227595,"number":121,"title":"MIST-589 Test all the things","user":{"login":"TauZero","id":752701,"avatar_url":"https://avatars.githubusercontent.com/u/752701?v=3","gravatar_id":"","url":"https://api.github.com/users/TauZero","html_url":"https://github.com/TauZero","followers_url":"https://api.github.com/users/TauZero/followers","following_url":"https://api.github.com/users/TauZero/following{/other_user}","gists_url":"https://api.github.com/users/TauZero/gists{/gist_id}","starred_url":"https://api.github.com/users/TauZero/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/TauZero/subscriptions","organizations_url":"https://api.github.com/users/TauZero/orgs","repos_url":"https://api.github.com/users/TauZero/repos","events_url":"https://api.github.com/users/TauZero/events{/privacy}","received_events_url":"https://api.github.com/users/TauZero/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":null,"milestone":null,"comments":2,"created_at":"2016-01-06T17:52:08Z","updated_at":"2016-02-02T21:19:26Z","closed_at":null,"pull_request":{"url":"https://api.github.com/repos/mistifyio/lochness/pulls/121","html_url":"https://github.com/mistifyio/lochness/pull/121","diff_url":"https://github.com/mistifyio/lochness/pull/121.diff","patch_url":"https://github.com/mistifyio/lochness/pull/121.patch"},"body":"Tests for lochness, the sub packages, internal packages, and commands.  A small handful of commands were left out of testing due to how thin they were (e.g. cli tools that are just wrappers) or very difficult testing.\n\n<!-- Reviewable:start -->\n[<img src=\"https://reviewable.io/review_button.png\" height=40 alt=\"Review on Reviewable\"/>](https://reviewable.io/reviews/mistifyio/lochness/121)\n<!-- Reviewable:end -->\n"},"comment":{"url":"https://api.github.com/repos/mistifyio/lochness/issues/comments/178826410","html_url":"https://github.com/mistifyio/lochness/pull/121#issuecomment-178826410","issue_url":"https://api.github.com/repos/mistifyio/lochness/issues/121","id":178826410,"user":{"login":"mmlb","id":708570,"avatar_url":"https://avatars.githubusercontent.com/u/708570?v=3","gravatar_id":"","url":"https://api.github.com/users/mmlb","html_url":"https://github.com/mmlb","followers_url":"https://api.github.com/users/mmlb/followers","following_url":"https://api.github.com/users/mmlb/following{/other_user}","gists_url":"https://api.github.com/users/mmlb/gists{/gist_id}","starred_url":"https://api.github.com/users/mmlb/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/mmlb/subscriptions","organizations_url":"https://api.github.com/users/mmlb/orgs","repos_url":"https://api.github.com/users/mmlb/repos","events_url":"https://api.github.com/users/mmlb/events{/privacy}","received_events_url":"https://api.github.com/users/mmlb/received_events","type":"User","site_admin":false},"created_at":"2016-02-02T21:19:25Z","updated_at":"2016-02-02T21:19:25Z","body":"\n\n\n\nReviewed 87 of 87 files at r1.\nReview status: all files reviewed at latest revision, 16 unresolved discussions.\n\n---\n\n*[cmd/common_test/cmd.go, line 19 \\[r1\\]](https://reviewable.io:443/reviews/mistifyio/lochness/121#-K9V-SLL3YgMGX1xmFQF:-K9V-SLL3YgMGX1xmFQG:-1761055888) ([raw file](https://github.com/mistifyio/lochness/blob/cfc7ab40462bcad803103f725b0a60f2cd3b25f4/cmd/common_test/cmd.go#L19)):*\nThis is a misleading name, exec in the unix world has a very well defined behavior mainly that this process's memory is replaced with the exec'ed one, which is not at all what is going on here. Why not `Start`? It actually matches the stdlib term and what you are doing too.\n\n---\n\n*[cmd/common_test/ct.go, line 39 \\[r1\\]](https://reviewable.io:443/reviews/mistifyio/lochness/121#-K9UpgEZNELR81agFhcJ:-K9UpgEZNELR81agFhcK:-1681784080) ([raw file](https://github.com/mistifyio/lochness/blob/cfc7ab40462bcad803103f725b0a60f2cd3b25f4/cmd/common_test/ct.go#L39)):*\ndelete\n\n---\n\n*[cmd/common_test/ct.go, line 68 \\[r1\\]](https://reviewable.io:443/reviews/mistifyio/lochness/121#-K9UpeCzUGEu4y0_HlSH:-K9UpeCzUGEu4y0_HlSI:-1681784080) ([raw file](https://github.com/mistifyio/lochness/blob/cfc7ab40462bcad803103f725b0a60f2cd3b25f4/cmd/common_test/ct.go#L68)):*\ndelete\n\n---\n\n*[cmd/common_test/ct.go, line 120 \\[r1\\]](https://reviewable.io:443/reviews/mistifyio/lochness/121#-K9UqHBbQQDXxAh69YQa:-K9UqHBbQQDXxAh69YQb:-427504162) ([raw file](https://github.com/mistifyio/lochness/blob/cfc7ab40462bcad803103f725b0a60f2cd3b25f4/cmd/common_test/ct.go#L120)):*\nwhat's special about 4066?\n\n---\n\n*[cmd/common_test/README.md, line 3 \\[r1\\]](https://reviewable.io:443/reviews/mistifyio/lochness/121#-K9Uy0OGQXLPM-E6OTMs:-K9Uy0OGQXLPM-E6OTMt:1486407715) ([raw file](https://github.com/mistifyio/lochness/blob/cfc7ab40462bcad803103f725b0a60f2cd3b25f4/cmd/common_test/README.md#L3)):*\nI have a few nits with this package.\n\n1. Can we move `common_test` out of cmd dir? This makes `go intall github.com/mistifyio/lochness/...` pollute `$GOPATH/bin` with non-useful binaries. We could possibly move to something like `lochness/test/common`.\n\n2. -The New{Guest,Hypervisor,...} functions seem to have a bunch of overlap with `lochness/testhelper` func that `cdhcpd` uses. We should probably merge them.- (kept comment for reference)\n\n3. I kind of alluded to it in 1, but having a package name not match the dir name is a bit of a go nono, either name the dir `ct` or rename the package `common_test`, I think `lochness/test/common` is nice because then the funcs would be `common.TestSuite` and `foo.TestSuite.SetupSuite()` for example, seems nicer.\n\n4. Also since all the package tests are not part of the package maybe we should move to `tests/lochness`? This will cut down on the number of files in first dir that new people will have to look at.\n\n---\n\n*[cmd/common_test/README.md, line 9 \\[r1\\]](https://reviewable.io:443/reviews/mistifyio/lochness/121#-K9V07gMZRJq9BZffoQ9:-K9V07gN8bp5cyiFssZH:-985174421) ([raw file](https://github.com/mistifyio/lochness/blob/cfc7ab40462bcad803103f725b0a60f2cd3b25f4/cmd/common_test/README.md#L9)):*\nhmm I wonder if this README is in need of regeneration? Aren't the godoc comments supposed to show up here too? So it seems that the template's `{{ if not .IsCommand }}` is matching this as a command and therefore not generating the usage info. That seems like a bug in godocdown to me.\n\n---\n\n*[cmd/cworkerd/main.go, line 30 \\[r1\\]](https://reviewable.io:443/reviews/mistifyio/lochness/121#-K9V1Azj1Kyk6hygH-vU:-K9V1Azj1Kyk6hygH-vV:1118419584) ([raw file](https://github.com/mistifyio/lochness/blob/cfc7ab40462bcad803103f725b0a60f2cd3b25f4/cmd/cworkerd/main.go#L30)):*\nMan I really hate it when tool defaults aren't really specified and I have to go digging for it. I think you should just replace `0` with the actual default or at least document it in the help message.\n\n---\n\n*[common_test.go, line 29 \\[r1\\]](https://reviewable.io:443/reviews/mistifyio/lochness/121#-K9V40YKoctIfnEbRnJS:-K9V40YKoctIfnEbRnJT:-1681784080) ([raw file](https://github.com/mistifyio/lochness/blob/cfc7ab40462bcad803103f725b0a60f2cd3b25f4/common_test.go#L29)):*\ndelete\n\n---\n\n*[common_test.go, line 77 \\[r1\\]](https://reviewable.io:443/reviews/mistifyio/lochness/121#-K9V49KHgu8H8XWVavlb:-K9V49KHgu8H8XWVavlc:272928920) ([raw file](https://github.com/mistifyio/lochness/blob/cfc7ab40462bcad803103f725b0a60f2cd3b25f4/common_test.go#L77)):*\n???? I'm confused, why is this and the others below here when we also have `PrefixKey`?\n\n---\n\n*[config.go, line 52 \\[r1\\]](https://reviewable.io:443/reviews/mistifyio/lochness/121#-K9UoEa0TrZk-JmeVP5F:-K9UoEa0TrZk-JmeVP5G:-210542507) ([raw file](https://github.com/mistifyio/lochness/blob/cfc7ab40462bcad803103f725b0a60f2cd3b25f4/config.go#L52)):*\nThere is enough similarity with `Context.ForEachConfig` that maybe they should be merged or share a sub-function?\n\n---\n\n*[hypervisor.go, line 311 \\[r1\\]](https://reviewable.io:443/reviews/mistifyio/lochness/121#-K9V5fPYXnf4N1Uc5doK:-K9V5fPYXnf4N1Uc5doL:1862950695) ([raw file](https://github.com/mistifyio/lochness/blob/03e0eae96254b79339580d5dc0ae99dfb20aae92/hypervisor.go#L311)):*\ngutes wtf?\n\n---\n\n*[hypervisor_test.go, line 200 \\[r1\\]](https://reviewable.io:443/reviews/mistifyio/lochness/121#-K9YPOzQBlbTr4DpMo2t:-K9YPOzRHRhve4S15eY2:1730760690) ([raw file](https://github.com/mistifyio/lochness/blob/cfc7ab40462bcad803103f725b0a60f2cd3b25f4/hypervisor_test.go#L200)):*\nYou can collapse these 2 lines into just\n```\nclobberHypervisor := *goodHypervisor\n```\n\nor even just use `*goodHypervisor` directly in the table/test def.\n\n---\n\n*[hypervisor_test.go, line 354 \\[r1\\]](https://reviewable.io:443/reviews/mistifyio/lochness/121#-K9YQo02RjkVFdWpiTMS:-K9YQo02RjkVFdWpiTMT:452109424) ([raw file](https://github.com/mistifyio/lochness/blob/cfc7ab40462bcad803103f725b0a60f2cd3b25f4/hypervisor_test.go#L354)):*\nrename to `guest1`?\n\n---\n\n*[network_test.go, line 103 \\[r1\\]](https://reviewable.io:443/reviews/mistifyio/lochness/121#-K9YmICRIxVIJv9c0TaM:-K9YmICRIxVIJv9c0TaN:510474628) ([raw file](https://github.com/mistifyio/lochness/blob/cfc7ab40462bcad803103f725b0a60f2cd3b25f4/network_test.go#L103)):*\ncan do the same as in hypervisor testszn\n\n---\n\n*[pkg/deferer/deferer.go, line 84 \\[r1\\]](https://reviewable.io:443/reviews/mistifyio/lochness/121#-K9YmUjj2xd9z4R7v5EW:-K9YmUjj2xd9z4R7v5EX:780942676) ([raw file](https://github.com/mistifyio/lochness/blob/cfc7ab40462bcad803103f725b0a60f2cd3b25f4/pkg/deferer/deferer.go#L84)):*\nthis is redundant\n\n---\n\n*[pkg/jobqueue/README.md, line 97 \\[r1\\]](https://reviewable.io:443/reviews/mistifyio/lochness/121#-K9YnhFJeKw_MEsmxZ9k:-K9YnhFJeKw_MEsmxZ9l:-1889122752) ([raw file](https://github.com/mistifyio/lochness/blob/cfc7ab40462bcad803103f725b0a60f2cd3b25f4/pkg/jobqueue/README.md#L97)):*\ntypo in source?\n\n---\n\n\n*Comments from the [review on Reviewable.io](https://reviewable.io:443/reviews/mistifyio/lochness/121)*\n<!-- Sent from Reviewable.io -->\n"}}