small nits, rest seems alright

I personally hate dockerCmd, so I'm strongly -1 on this. It hides real source of error inside dockerCmd.

@LK4D4 when you say "real source", do you mean the location of the source file (...test.go) that had the bad cmd?

@duglin I mean code line too.

I like it when the cmd you are running is more of a test setup than the actual thing your testing.
So if it does fail then there is something else majorly wrong.

I don't like passing *check.C anywhere from tests. It is pretty easy to reduce codebase with asserts now without such things.

@LK4D4 I was just wondering if it would help to have dockerCmd use reflection to figure out the test/line it was called from? I don't have a strong feeling either way but it is kind of nice to not have to add if-statements/Asserts over and over if we can move 'em all into the util func.

@duglin I don't feel nice about reading and debugging reflect-code when I can just see where really error was occured. I had problems with that before in integration-cli helpers and I prefer readability over writability and I personally don't care about if err != nil or c.Assert.

I personally don't like it either, so before rebasing this, shall we close this and maybe create an issue to remove dockerCmd* entirely from tests? ping @duglin @LK4D4

Actually, go-check does report the correct caller for you.

I'm probably in the minority here but I don't have a problem with utility functions doing more than setup work. They are certainly good for that but I also view utility functions as things to make writing testcases easier. So, moving code that appears in a lot of tests into a func to save people from having to duplicate that code over and over is ok with me - even if that func is the one doing the functional testing or doing the c.Fatal() call (as long as it reports the proper info so people know which test to look at to see what was being tested).

But I won't -1 removing it since its not that big of an issue - just more replicated code in the test.

alright, I think that now that there is gocheck most of the err checking and fataling is handled with a one liner c.Assert. In the end dockerCmd* are just pseudo proxies functions that are just for either not checking the error upstream or setting exec.Command dir or running with a timeout. I think we should have just one standard way of doing all of this otherwise it could be confusing which way to use (other than personal like). I vote for removing dockerCmd* 😄 and have just the runCommand* functions handling everything

heh, I'd say runCommand and runCommandWithOutput are much more superfluous, since these are literally just cmd.Run() and cmd.CombinedOutput(), respectively.

dockerCmd actually provides a level of abstraction.
One might even want something like id := createContainer(), and remove all theses strings.TrimSpace, and custom container names.

ok, I just like to have just one way of doing things, above all in tests suite where it's super easy to just implement functions for some use cases and then just forget. Instead a clean interface will make things clear immediately on how to run a container, a container with output etc etc

@runcom @cpuguy83 Btw I noticed that gocheck actually resolves wrong caller problem. So, I'm okay with this if we'll change it to use c.Assert

@fntlnz could you please rebase and have dockerCommand using c.Assert?

Yeah I'll do all this evening along with the other pr assigned to me.

:)
On May 19, 2015 7:11 PM, "Antonio Murdaca" notifications@github.com wrote:

@fntlnz https://github.com/fntlnz could you please rebase and have
dockerCommand using c.Assert?

—
Reply to this email directly or view it on GitHub
#12838 (comment).

@runcom @LK4D4 looks good to you?

LGTM

ping @runcom

LGTM. Merging 🎉