Namespace in unit tests

[TAMURA, Kent]

It seems namespace usage in unit tests is inconsistent.

A) No namespace

  Example: base/atomicops_unittest.cc

B) Wrap everything with anonymous namespace

  Example: base/barrier_closure_unittest.cc

C) Wrap everything with the namespace same as the target code

  Example: base/big_endian_unittest.cc

What's recommended?

Google C++ style guid doesn't mention this.

A Googletest document mentions that C is required to use FRIEND_TEST macro.

https://code.google.com/p/googletest/wiki/AdvancedGuide#Private_Class_Members

--

TAMURA Kent
Software Engineer, Google

[Viet-Trung Luu]

On Tue, Apr 21, 2015 at 6:10 PM, TAMURA, Kent <tk...@chromium.org> wrote:

It seems namespace usage in unit tests is inconsistent.

A) No namespace

  Example: base/atomicops_unittest.cc

B) Wrap everything with anonymous namespace

  Example: base/barrier_closure_unittest.cc

C) Wrap everything with the namespace same as the target code

  Example: base/big_endian_unittest.cc

What's recommended?

My personal preference is to combine B) and C), e.g.,

namespace foo {

namespace {

...

The namespace foo to avoid having to say foo:: everywhere, and the anonymous namespace to avoid inadvertent ODR violations (which can lead to strange, hard-to-debug issues).

The risk of ODR violations is especially high if you have helper functions/classes. Of course, you can just put those in an anonymous namespace (or declare functions static), but I find that having an anonymous namespace wrapping everything allows for better organization of the file (e.g., if you have BarHelper(), you can naturally define it immediately above TEST(FooTest, Bar)).

 

Google C++ style guid doesn't mention this.

True. (Note that ODR violations due to tests are less of a concern in environments where people don't dump all their tests into huge test binaries.)

 

A Googletest document mentions that C is required to use FRIEND_TEST macro.

https://code.google.com/p/googletest/wiki/AdvancedGuide#Private_Class_Members

This is true, unfortunately. :-/

 

--

TAMURA Kent
Software Engineer, Google

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

To unsubscribe from this group and stop receiving emails from it, send an email to chromium-dev...@chromium.org.

[Peter Kasting]

On Tue, Apr 21, 2015 at 6:10 PM, TAMURA, Kent <tk...@chromium.org> wrote:

It seems namespace usage in unit tests is inconsistent.

A) No namespace

  Example: base/atomicops_unittest.cc

B) Wrap everything with anonymous namespace

  Example: base/barrier_closure_unittest.cc

C) Wrap everything with the namespace same as the target code

  Example: base/big_endian_unittest.cc

If you're testing code in a namespace, prefer (C) to the combination of "(A) and using explicit qualifiers everywhere".  If (C) doesn't actually save you any qualifiers, don't bother with it.

I tend to like (B) for all helpers and the like, and then leave just the TEST(...) {} functions at the bottom of the file outside the anon namespace.  This is basically consistent with how I'd write a C++ file, where any file-scope helpers are in an anon namespace and the "main code" is not.  Technically the TEST parts could be namespaced as well, I just don't because it seems harmless not to.

PK

[Yoshifumi Inoue]

It seems my patch for Blink is the root cause of this thread. (^_^;)

In Blink repository, 134 files are wrap everything in anonymous namespace and have "using namespace blink".

My concern is "using namespace" directive isn't match Google C++ coding style guide:

https://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Namespaces

-yosi

P.S. My preference is also Viet's:

namespace foo {

namespace {

...

2015年4月22日水曜日 10時49分43秒 UTC+9 Peter Kasting:

[Peter Kasting]

On Tue, Apr 21, 2015 at 7:11 PM, Yoshifumi Inoue <yo...@chromium.org> wrote:

In Blink repository, 134 files are wrap everything in anonymous namespace and have "using namespace blink".

My concern is "using namespace" directive isn't match Google C++ coding style guide:

https://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Namespaces

You're correct, that's a style guide violation.

PK 

[Yutaka Hirano]

+1 for Viet's idea.

Checking that the entire code is inside an anonymous namespace is easier than checking that all helpers are in an anonymous namespace.

--

[TAMURA, Kent]

I found we had the same topic three years ago:

https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/MwPIo2BnVhM

[Peter Kasting]

On Tue, Apr 21, 2015 at 10:09 PM, TAMURA, Kent <tk...@chromium.org> wrote:

I found we had the same topic three years ago:

https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/MwPIo2BnVhM

Huh.  The short summary: don't put your TESTs in an anonymous namespace.

The FRIEND_TEST (and "friend" declarations in general) thing is something I should have thought of, though it's also something that people can easily exclude tests from anonymous namespaces on a case-by-case basis for.  I didn't know about the globally unique testnames rule though.  Interesting.

PK

[Dana Jansens]

On Tue, Apr 21, 2015 at 6:28 PM, Viet-Trung Luu <viettr...@chromium.org> wrote:

On Tue, Apr 21, 2015 at 6:10 PM, TAMURA, Kent <tk...@chromium.org> wrote:

It seems namespace usage in unit tests is inconsistent.

A) No namespace

  Example: base/atomicops_unittest.cc

B) Wrap everything with anonymous namespace

  Example: base/barrier_closure_unittest.cc

C) Wrap everything with the namespace same as the target code

  Example: base/big_endian_unittest.cc

What's recommended?

My personal preference is to combine B) and C), e.g.,

namespace foo {

namespace {

...

+1 this is what I do/ask people to do also.

[Jeremy Roman]

+1 except when friendship is required.

[TAMURA, Kent]

Thank you for comments.

Viet's way is reasonable.

I'll recommend in code review:

* Wrap test code with the namespace same as test target code.

  - We can omit foo:: in test code.

  - Necessary for FRIEND_TEST

    (Note that we should use FRIEND_TEST_ALL_PREFIXES actually.)

* Wrap test code with anonymous namespace additionally, like |namespace foo {\nnamespace {\n|.

  - it's recommended for helper functions/classes to avoid ODR violation.

  - We shouldn't wrap test fixture class and tests declared in FRIEND_TEST.

  - We may wrap test fixture class and tests without FRIEND_TEST.

On Wed, Apr 22, 2015 at 10:10 AM, TAMURA, Kent <tk...@chromium.org> wrote:

[Peter Kasting]

On Wed, Apr 22, 2015 at 5:26 PM, TAMURA, Kent <tk...@chromium.org> wrote:

* Wrap test code with anonymous namespace additionally, like |namespace foo {\nnamespace {\n|.

This seems to violate the suggestions on the linked-to previous discussion thread, where people were asked to never anon-namespace their TESTs, so that duplicate test names would cause a link-time failure instead of a runtime failure.

PK

[TAMURA, Kent]

You're right.

I tested locally.

 - If I had duplicated test names not in anonymous namespaces, I had link errors.

 - If I had duplicated test names in anonymous namespaces, I had no link errors and no runtime errors on running the test binary.  --gtest_filter=<the duplicated name> looked to run both of the tests.  It's confusing.

So, I correct as follows:

* Wrap helper functions/classes in test files with anonymous namespace additionally, like |namespace foo {\nnamespace {\n|.

  - it's recommended to avoid ODR violation.

  - We shouldn't wrap test fixture class and tests

[Thiago Farina]

On Wed, Apr 22, 2015 at 10:41 PM, TAMURA, Kent <tk...@chromium.org> wrote:

You're right.

I tested locally.

 - If I had duplicated test names not in anonymous namespaces, I had link errors.

 - If I had duplicated test names in anonymous namespaces, I had no link errors and no runtime errors on running the test binary.  --gtest_filter=<the duplicated name> looked to run both of the tests.  It's confusing.

So, I correct as follows:

* Wrap helper functions/classes in test files with anonymous namespace additionally, like |namespace foo {\nnamespace {\n|.

  - it's recommended to avoid ODR violation.

  - We shouldn't wrap test fixture class and tests

So a foo_test.cc should look like the following?

foo_test.cc

#include "path/to/foo.h"

#include "..."

namespace my_module {

namespace {

// helper functions/classes

}  // namespace

TEST(FooTest, CaseName) {

}

...

}  // namespace my_module 

--

Thiago Farina

[Yutaka Hirano]

We have the same problem with tests in different namespaces (e.g. base vs. base::i18n), so forbidding anonymous namespace doesn't solve the problem.

[Peter Kasting]

On Wed, Apr 22, 2015 at 8:56 PM, Yutaka Hirano <yhi...@google.com> wrote:

We have the same problem with tests in different namespaces (e.g. base vs. base::i18n), so forbidding anonymous namespace doesn't solve the problem.

Correct, but since most Chrome code isn't currently namespaced, that would mean by default most TESTs wouldn't be namespaced either, whereas if we always wrap everything in anonymous namespaces, they would.  So I think it would make a material difference to the chances of encountering the problem.

Honestly, I don't think it's a huge deal.  If someone really wants to include namespaces or really wants to leave them out, I think we should let them.  If people want guidance, then this thread suggests some.

PK 

[Ben]

Alternative to the FRIEND_TEST and FRIEND_TEST_ALL_PREFIXES you can friend a TestAPI class.  Example here: shell_test_api.h

I like this approach better because it is more scalable, you don't need to keep adding FRIEND_TEST_ALL_PREFIXES for each and every test.

[TAMURA, Kent]

Wrapping everything with anonymous namespace is simple and acceptable, I think.  Test name conflict is a minor problem.

When I write a test, I wrap only helper functions/classes with anonymous namespace. However, I'll say lgtm for CLs wrapping test fixtures and test functions with anonymous namespace.

[TAMURA, Kent]

Yes, exactly.

[Yutaka Hirano]

I see, thanks.