Death to #ifdef UNIT_TEST?

[Darin Fisher]

Ricardo and I were chatting about the UNIT_TEST macro, and we were considering killing it.  Actually, as we've been converting code over to support being built as component DLLs, we've been killing the use of the UNIT_TEST macro.  It gets in the way of building DLLs, although it is possible to workaround that issue if we really wanted to.  Hence, this email.

I'm wondering if people see much value to using this macro.  Typical usage is like this:

class Foo {

 public:

  ...

#ifdef UNIT_TEST

  void DoSomethingThatOnlyUnitTestsShouldBeAbleToDo() {

    // Implemented inline so we can link.

    ...

  }

#endif

  ...

};

There is not too much of this in the code base:

http://codesearch.google.com/#search/&exact_package=chromium&q=%22%23ifdef%20UNIT_TEST%22&type=cs

http://codesearch.google.com/#search/&exact_package=chromium&q=%22%23if%20defined(UNIT_TEST)%22&type=cs

I can discuss how to make this work for DLLs if people are interested.

Thoughts?

-Darin

[Peter Kasting]

On Thu, Aug 4, 2011 at 1:07 PM, Darin Fisher <da...@chromium.org> wrote:

I'm wondering if people see much value to using this macro.

I've certainly used it.  It can be a nice alternative to FRIEND_TEST_xxx if a lot of tests have to access something but you don't want to expose it in general.  So if it's easy to change it to a different format that works for you, I'd be for that.  But if it's a big hassle, we can kill this -- there are other solutions, at worst exposing some function to all callers and adding a comment saying only tests should call it.

PK 

[Adam Barth]

or a runtime check that enforces the comment:

#define UNIT_TEST_ONLY CHECK(g_running_unit_tests);

where g_running_unit_tests is a global static that defaults to false
and that the unit testing framework sets to true.

Adam

[Paweł Hajdan, Jr.]

I'd prefer to avoid adding any new globals to our testing infrastructure.

I think UNIT_TEST may be useful sometimes, and I have a similar opinion to Peter's: it's a good alternative to friending tests, but if it's too problematic to support I'm fine with removing it.

A possible alternative approach to enforce that only tests can use selected methods would be a Clang-based checker.

--
Chromium Developers mailing list: chromi...@chromium.org
View archives, change email options, or unsubscribe:
   http://groups.google.com/a/chromium.org/group/chromium-dev

[Ami Fischman]

or a runtime check that enforces the comment:

#define UNIT_TEST_ONLY CHECK(g_running_unit_tests);
where g_running_unit_tests is a global static that defaults to false
and that the unit testing framework sets to true.

FWIW, gtest's related advice justifying why it doesn't provide such a facility: http://code.google.com/p/googletest/wiki/FAQ#How_can_my_code_detect_if_it_is_running_in_a_test?

(of course some of the concerns there are mitigated if you only use this to CHECK-fail, but slippery slope and so forth)

-a

[Darin Fisher]

On Thu, Aug 4, 2011 at 1:11 PM, Peter Kasting <pkas...@chromium.org> wrote:

The alternative solution looks like this:

#if defined(UNIT_TEST) || defined(FOO_IMPLEMENTATION)

  ... code ...

#endif

FOO_IMPLEMENTATION is defined when compiling the Foo component (e.g., NET_IMPLEMENTATION).  The deal here is that if the code is a member function of a class that is being exported from the Foo component, then it is necessary to also export the code that the unit test needs.

However, instead of doing the above, what I have been doing is renaming the member functions.  Under src/views/, instead of GetNativeWrapper(), we have GetNativeWrapperForTesting(), and so on.  Yucky?

-Darin

[Jói Sigurðsson]

> However, instead of doing the above, what I have been doing is renaming the
> member functions.  Under src/views/, instead of GetNativeWrapper(), we have
> GetNativeWrapperForTesting(), and so on.  Yucky?

Seems OK to me, and the least complicated approach. We could write a
presubmit check to ensure functions with names ending with ForTesting
are only called from _unittest or _test_support files.

Cheers,
Jói

[Albert J. Wong (王重傑)]

I've previously seen a number of "XXXForTest()" functions, and this
crude search makes it look relatively prevalent.

http://google.com/codesearch#search/&q=.*ForTest%5C(.*%5C)%5C%20%7B%20file:.*%5C.cc&type=cs

Seems more popular that the ForTesting variant.

http://google.com/codesearch#search/&q=.*ForTesting%5C(.*%5C)%5C%20%7B%20file:.*%5C.cc&type=cs

Personally, having the function named "ForTest" seems like pretty
strong documentation even without any sort of build system
enforcement.

-Albert

[Darin Fisher]

Yes, I like making it self-documenting at the callsite.  It should be pretty obvious to coders and reviewers that adding a call to a ForTest{ing} function from non-test code is probably not right.  I don't think we have any evidence of this being abused, and I'd like to wait until we do before adding further complexity.

joi's suggested presubmit test idea seems good.  We might need to expand the set of file suffixes though.  Maybe _{browser,ui,perf}test.cc should be included at least.

-Darin

[Jói Sigurðsson]

I'll write the presubmit check tomorrow morning.

Cheers,
Jói

[Darin Fisher]

Thanks!!