Added argument to setUp and tearDown functions containing name of the…

[jstourac]

… relevant test

[rugk]

I think that is a nice addition, @kward, is not it?

[jstourac]

Well, I have rebased this one. Let me know whether there is anything I could improve here. Thx.

[kward]

I feel this is not a good addition for a couple of reasons.

• The addition of an argument to setup() or tearDown() makes these functions different than other unit test frameworks out there. While the argument is optional, it breaks the consistency one tends to expect for such functions.
• If the argument were added, the result would be developers using that argument to make programatic decisions in the setup() or tearDown() functions. A widely held philosophy of unit tests is that they should only contain enough code to demonstrate the test, and no more. Any usage of that argument for results in additional code being written, code that itself should be tested, which violates the principle I mentioned earlier.

If the desired behavior is to have the setup() function behave differently according to what test was called, it would be better to create custom functions that do the work needed, and call them at the start of each test. For example, one might create a doSomething() function and call it from tests that need it.

doSomething() { echo "doSomething() called" >&2; }
# Tests #1 and #2 call doSomething because they need it.
test1() { doSomething; assertTrue ${SHUNIT_TRUE}; }
test2() { doSomething; assertFalse ${SHUNIT_FALSE}; }
# Test #3 does not call doSomething because it doesn't need it.
test3() {assertEquals 1 1; }
source shunit2

Additionally, individual unit tests should only call functions from within the test that setup the environment in some way. Tests should not call cleanup functions. The reasoning is that the cleanup function may never be called due to a failure in the test. Instead, the tearDown() function should do any necessary cleanup, even if cleanup is not needed, to ensure that cleanup is done.

[jstourac]

Actually, we use this change just to properly log name of the test that is being executed for our test harness logging mechanism. Also, this change preserved original setUp() and tearDown() functions without arguments, thus no API is broken, actually, for those people who are used to standard declaration of those two functions.

Although, I understand your concerns regarding to the possibility to misuse such information for some decision making process. I just saw this as the easiest and straigforward solution for our problem and though it might be useful also for somebody else.

[kward]

If you are looking to log that info, I think a better option would be to add logging hooks that are disabled by default (just like setup() and tearDown()) unless somebody overrides them. We could pass the function name to them, which would not break the API for the existing functions, and would give you the ability to define those hooks however you like.

I'd do it just before calling setup() https://github.com/kward/shunit2/blob/master/shunit2#L885

and just after tearDown() https://github.com/kward/shunit2/blob/master/shunit2#L899

Thoughts? If you like that idea, would you want to code that up?

[kward]

Maybe a single logging hook, where you pass to it what phase you are logging, as well as the function name?

[virajkarandikar]

@jstourac , I see that $shunit_test is already visible in setUp()