FRIEND_TEST_ALL_PREFIXES vs Peer classes

[Jochen Eisinger]

Hey,

the following question came up in a review. Should we prefer introducing Peer classes as described in https://abseil.io/tips/135 over usage of FRIEND_TEST_ALL_PREFIXES (for the reasons mention in said blog post), or should we continue using FRIEND_TEST_ALL_PREFIXES because consistency.

best

-jochen

[Daniel Cheng]

I think using adding a helper class that's a friend of the tested class is a lot cleaner than using FRIEND_TEST / FRIEND_TEST_ALL_PREFIXES, since the list of friend tests can get quite long (and it introduces this weird coupling of production code on test code). I think consistency won't be too bad: certainly, an individual test file should be internally consistent, but using this in new test files seems like a good idea to me.

I wonder why their recommendation steers clear of just having the class friend the test fixture: I guess the idea is the helper class could be shared between multiple unittest modules that way?

Daniel

--
You received this message because you are subscribed to the Google Groups "cxx" group.
To unsubscribe from this group and stop receiving emails from it, send an email to cxx+uns...@chromium.org.
To post to this group, send email to c...@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CALjhuifVPTPNYaKK20o4runexdpwnzFWjix9T%2B-s2yOHL_ETzw%40mail.gmail.com.

[Martin Kreichgauer]

On Tue, Jul 31, 2018 at 10:56 AM Daniel Cheng <dch...@chromium.org> wrote:

I think using adding a helper class that's a friend of the tested class is a lot cleaner than using FRIEND_TEST / FRIEND_TEST_ALL_PREFIXES, since the list of friend tests can get quite long (and it introduces this weird coupling of production code on test code). I think consistency won't be too bad: certainly, an individual test file should be internally consistent, but using this in new test files seems like a good idea to me.

I wonder why their recommendation steers clear of just having the class friend the test fixture: I guess the idea is the helper class could be shared between multiple unittest modules that way?

The Google style guide does say "In some cases it may be useful to make a unittest class a friend of the class it tests.". IMHO, it depends on the circumstances. A test peer can make it more obvious, which implementation details are being exposed, and it can be reused between different test fixtures.

[Peter Kasting]

On Tue, Jul 31, 2018 at 10:56 AM Daniel Cheng <dch...@chromium.org> wrote:

I think using adding a helper class that's a friend of the tested class is a lot cleaner than using FRIEND_TEST / FRIEND_TEST_ALL_PREFIXES, since the list of friend tests can get quite long (and it introduces this weird coupling of production code on test code). I think consistency won't be too bad: certainly, an individual test file should be internally consistent, but using this in new test files seems like a good idea to me.

I wonder why their recommendation steers clear of just having the class friend the test fixture: I guess the idea is the helper class could be shared between multiple unittest modules that way?

Your comments sound like they still center on the idea that the test tests private details of the original class, and just looks at differences in how that could be made to occur.

It sounds to me like instead this TOTW is trying to request that tests not access implementations' private methods/variables, and that instead the class be structured in such a way that the public API is sufficient for the relevant testing.  This encourages authors to test the publicly-visible behavior instead of the fine-grained details of how that public behavior is implemented at a low level, thus making tests less brittle.

PK

[Daniel Cheng]

I agree that not accessing the private details is better, when possible. But in the case that's not possible, using a friend class seems strictly better than FRIEND_TEST / FRIEND_TEST_ALL_PREFIXES.

Daniel

[Peter Kasting]

On Tue, Jul 31, 2018 at 11:22 AM Daniel Cheng <dch...@chromium.org> wrote:

I agree that not accessing the private details is better, when possible. But in the case that's not possible, using a friend class seems strictly better than FRIEND_TEST / FRIEND_TEST_ALL_PREFIXES.

I think reasonable arguments could be made for either position; to me, if there's anything to debate here, it's more the core idea of "try as hard as possible to not test private details", which is very much not the current philosophy in Chrome code.  The specifics of how to test private details if you decide to do so feels a little more bikeshed-y.

PK 

[Daniel Cheng]

FRIEND_TEST_ALL_PREFIXES is overused and clutters non-test code. There are 82 files with 10+ instances FRIEND_TEST_ALL_PREFIXES, with the record being 50 in this file: https://cs.chromium.org/chromium/src/components/safe_browsing/db/v4_store.h?rcl=017b67704ac5c26b60a3674866a2325c8db020cd&l=230

Daniel

[Martin Kreichgauer]

I think everyone agrees that testing private implementation details should be avoided. Still, the ToTW IMO makes a strong case that the difference between friending a peer class (or maybe a test fixture) and using FRIEND_TEST is not just bike shedding. Almost half of the post deals with that question.

[Peter Kasting]

On Tue, Jul 31, 2018 at 11:36 AM Martin Kreichgauer <mart...@google.com> wrote:

I think everyone agrees that testing private implementation details should be avoided.

Given the current state of Chromium code, I don't believe it's true that everyone agrees on this -- or we wouldn't be doing it so pervasively and consistently, including in new changes I review.

I think it is vastly more beneficial to our testability to actually get everyone to agree on this than to fix the issue of peer classes versus FRIEND_TEST.  The latter is a nice-to-have, the former is more must-have; and it's something that is not called out in our style guides or Dos-and-Don'ts page, or recommended to new team members anywhere.

PK

[Antoine Labour]

On Tue, Jul 31, 2018 at 11:51 AM 'Peter Kasting' via cxx <c...@chromium.org> wrote:

On Tue, Jul 31, 2018 at 11:36 AM Martin Kreichgauer <mart...@google.com> wrote:

I think everyone agrees that testing private implementation details should be avoided.

Given the current state of Chromium code, I don't believe it's true that everyone agrees on this -- or we wouldn't be doing it so pervasively and consistently, including in new changes I review.

Both black-box testing and white-box testing have their uses and trade-offs. I don't think it has to be one or the other.

Some complex classes may have internal invariants, and it is very reasonable and desirable to verify that those invariants are conserved when operating on the class, in unit tests. That requires exposing those invariants to the test, one way or the other.

Antoine

 

I think it is vastly more beneficial to our testability to actually get everyone to agree on this than to fix the issue of peer classes versus FRIEND_TEST.  The latter is a nice-to-have, the former is more must-have; and it's something that is not called out in our style guides or Dos-and-Don'ts page, or recommended to new team members anywhere.

PK

--

You received this message because you are subscribed to the Google Groups "cxx" group.
To unsubscribe from this group and stop receiving emails from it, send an email to cxx+uns...@chromium.org.
To post to this group, send email to c...@chromium.org.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAAHOzFBJSz-BDJ8QkFo-nB4VpKZfuMPMJxdftY2Gv_1V%3DjrXrQ%40mail.gmail.com.

[Jochen Eisinger]

so... what's the conclusion? Assuming that a given test wants to look at internal state, go with consistency, or go with the Peer pattern?

[Sadrul Chowdhury]

A few components in the tree use the peer-pattern, where the peers have a TestApi suffix. The usage is not super-consistent though (e.g. some are in a 'test' namespace, some not, some parts of the code in these components still use FRIEND_ macros). But I personally prefer this peer-pattern over using FRIEND_ in new code for the same reasons mentioned upthread.

Sadrul

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CALjhuicXa0FTN2MNYTRZFbg5VcF2TE5i%2BHH%2Bkosg3u%2BtYH_2GA%40mail.gmail.com.

[Albert J. Wong (王重傑)]

My vote is go for peer class rather than a forest of FRIEND_BLAHDEBLAH if the forest starts getting anywhere near big, but that it's okay to exercise judgement in this particular case.

Rigid consistency here doesn't seem to have huge benefits.

-Albert

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAKgcGXiV-SURitTmKfrvQiUKwfNecOXtMAga-jW3ZaYP6ytTnw%40mail.gmail.com.

[Victor Costan]

I think new code should use Peer classes.

As https://abseil.io/tips/ describes, the internal version of Abseil's tips of the week is referenced in Google's code reviews and has quite a bit of weight. While not the main subject of this thread, I think we should consider all the tips there and adopt them as strong recommendations, when they don't conflict with our own style guide and wisdom.

Asides from exposing a narrower interface to tests (which I really like), a hypothetical future where we get rid of all the FRIEND_ macros means we don't have to expose gtest_prod.h to non-test components. While I don't think these advantages are strong enough to migrate the whole codebase now, I do think they're significant enough to be worth adopting for new code.

    Victor

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CALcbsXAMQDrt1Mgnx3fp52VWWVOSZMNXg_%3DCP3EKiEd5AApTpQ%40mail.gmail.com.

[Jeremy Roman]

I agree with the totw (public members > peer classes > friending tests), but I'm not sure that I'd fight particularly hard for it in code review, especially while it's small and there's a small number of such tests.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAP_mGKrsOq2exZfY2uR4gsX2KtUb%3DT6%2Bs3hm9n8Y7q-7Ey%3D6%2Bg%40mail.gmail.com.