Style guide clarification for unit test naming

[Dominic Farolino]

In Chromium we often use the `_unittest.cc` suffix to distinguish unit tests from browser tests (typically in `_browsertest.cc` files).

However, the Google C++ style guide mentions that the `_unittest.cc` suffix for unit tests is deprecated, and the more-general `_test.cc` suffix should be used instead. Using `_test.cc` in Chromium can still distinguish unit tests from browser tests, although it is a lot less explicit in general, so I'm wondering what the Chromium-specific guidance here should be? I don't have a strong opinion, but most people I've talked to seems to still prefer using `_unittest.cc` to distinguish unit tests from browser tests, which makes sense to me. Regardless, I'd like to know:

• What others think about this
• If we decide to keep using `_unittest.cc`, is it worth adding something to the Chromium C++ style guide to clarify our divergence from the Google C++ style guide? I think so

Separately, there are many more instances of `_test.cc` in Blink than there are `_unittest.cc`, but this is suspected to be a holdover from the WebKit days. We have a separate style guide specifically for Blink, but I'd prefer having Blink and Chromium in general be aligned on whatever stance we take here. I suspect we'll resolve to using `_unittest.cc` suffixes, in which case it would make sense to leave the Blink style guide alone and I suppose rename all of the `_test.cc` files in Blink to use the `_unittest.cc` suffix. Do people have any thoughts on this?

Thanks,

Dom

[dan...@chromium.org]

I would like to avoid investing any engineer effort in renaming our unit test files, so I would suggest we stick with _unittest everywhere for consistency. Blink has been slowly moving toward consistency with the rest of Chrome already I believe.

Adding this to our style guide seems appropriate.

 

Thanks,

Dom

--
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 view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAP-uykB0woMo0bqnD2Y0Eo8Ck-LitgaMhMFMZr%3DpHsq31WxZEQ%40mail.gmail.com.

[Peter Kasting]

I'd like to avoid diverging from Google style on this, but I'd like even more not to say anything in the styleguide about something so trivial.  So, my preference is "name new files _test.cc, don't rename existing files, don't write anything in the styleguide, don't stress about violations".

PK

--

[dan...@chromium.org]

On Fri, Jul 2, 2021 at 3:26 PM 'Peter Kasting' via cxx <c...@chromium.org> wrote:

I'd like to avoid diverging from Google style on this, but I'd like even more not to say anything in the styleguide about something so trivial.  So, my preference is "name new files _test.cc, don't rename existing files, don't write anything in the styleguide, don't stress about violations".

The style guide does not mention browsertests but it does mention `_regtest`, and my reading of it would say that we should not use our browsertest suffix either then. Since we presumably want to keep that prefix and distinction (I think this is a key property of our testing systems and strategies), I think we should keep both. From there, stating this defacto rule somewhere in the guide does seem worthwhile to me, as it currently is left up to institutional knowledge, and noticing and copying existing code.

 

PK

On Fri, Jul 2, 2021 at 12:16 PM Dominic Farolino <d...@chromium.org> wrote:

In Chromium we often use the `_unittest.cc` suffix to distinguish unit tests from browser tests (typically in `_browsertest.cc` files).

However, the Google C++ style guide mentions that the `_unittest.cc` suffix for unit tests is deprecated, and the more-general `_test.cc` suffix should be used instead. Using `_test.cc` in Chromium can still distinguish unit tests from browser tests, although it is a lot less explicit in general, so I'm wondering what the Chromium-specific guidance here should be? I don't have a strong opinion, but most people I've talked to seems to still prefer using `_unittest.cc` to distinguish unit tests from browser tests, which makes sense to me. Regardless, I'd like to know:

• What others think about this
• If we decide to keep using `_unittest.cc`, is it worth adding something to the Chromium C++ style guide to clarify our divergence from the Google C++ style guide? I think so

Separately, there are many more instances of `_test.cc` in Blink than there are `_unittest.cc`, but this is suspected to be a holdover from the WebKit days. We have a separate style guide specifically for Blink, but I'd prefer having Blink and Chromium in general be aligned on whatever stance we take here. I suspect we'll resolve to using `_unittest.cc` suffixes, in which case it would make sense to leave the Blink style guide alone and I suppose rename all of the `_test.cc` files in Blink to use the `_unittest.cc` suffix. Do people have any thoughts on this?

Thanks,

Dom

--
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 view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAP-uykB0woMo0bqnD2Y0Eo8Ck-LitgaMhMFMZr%3DpHsq31WxZEQ%40mail.gmail.com.

--
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 view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAAHOzFAXcC%2Bq_YKji2sx5rOM6yjDv%2BTSTZ8Rs9%2BZ9GiyvHFFrw%40mail.gmail.com.

[Dominic Farolino]

On Fri, Jul 2, 2021 at 1:38 PM <dan...@chromium.org> wrote:

On Fri, Jul 2, 2021 at 3:26 PM 'Peter Kasting' via cxx <c...@chromium.org> wrote:

I'd like to avoid diverging from Google style on this, but I'd like even more not to say anything in the styleguide about something so trivial.  So, my preference is "name new files _test.cc, don't rename existing files, don't write anything in the styleguide, don't stress about violations".

The style guide does not mention browsertests but it does mention `_regtest`, and my reading of it would say that we should not use our browsertest suffix either then. Since we presumably want to keep that prefix and distinction (I think this is a key property of our testing systems and strategies), I think we should keep both. From there, stating this defacto rule somewhere in the guide does seem worthwhile to me, as it currently is left up to institutional knowledge, and noticing and copying existing code.

I think I agree with Dana here. I've uploaded a small CL to clarify this in the style guide, but I'm happy to hear any more objections or opinions.

[K. Moon]

This should be pretty easy to enforce with a presubmit check, right?

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAP-uykCxQL8%3DcVcmf9aDSUJJEDN_%2BEzCNfqhohycjCs2riqbxQ%40mail.gmail.com.

[dan...@chromium.org]

On Fri, Jul 2, 2021 at 4:35 PM K. Moon <km...@chromium.org> wrote:

This should be pretty easy to enforce with a presubmit check, right?

What are we enforcing? That browser tests and unittests are properly distinguished?

 

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CACwGi-6PNSk%2BinOuSa2YMmawkgRPHuJwh0YhmT6kPdwvM7kxyg%40mail.gmail.com.

[K. Moon]

Use of _unittest or _browsertest instead of just _test.

[dan...@chromium.org]

On Fri, Jul 2, 2021 at 4:58 PM K. Moon <km...@chromium.org> wrote:

Use of _unittest or _browsertest instead of just _test.

Ah ok, thanks. Yes we could, though it would prevent test helpers from ending in _test as well. Like these: https://source.chromium.org/search?ss=chromium%2Fchromium%2Fsrc&q=for_test.h

[Peter Kasting]

I don't want to keep _browsertest.cc either.

PK

[dan...@chromium.org]

I think that's a rather large change to make at this point, for no reason that I see other than because Google did it. That's a tough sell to me, but regardless, if we're going to make a change like that, probably cxx@ is not the place to decide.

[Peter Kasting]

The change would simply be not naming things that in the future, because doing so is deprecated. There's no need to rename existing files. As for why do so -- it's easier to find test code when it's always named the same, and it would save on renames when moving code between test suites (as had bitten me repeatedly before).

PK

[Avi Drissman]

For classes that have both unittests and browsertests, what would be the suggested file naming?

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

[Peter Kasting]

I don't know, how does google3 deal with multiple kinds of tests?

PK

[Peter Kasting]

On Fri, Jul 2, 2021 at 8:17 PM Peter Kasting <pkas...@google.com> wrote:

I don't know, how does google3 deal with multiple kinds of tests?

(Maybe we could #ifdef UNIT_TEST the unittests and have everything in one file?  That'd have saved a lot of helper code in certain cases in the past for me...)

For clarity: I don't care very much about this and I'm not going to push hard.  The thing I care most about is, style rules should pull their weight; think of each rule you add as fractionally decreasing the likelihood of all rules being remembered and followed.  Is this rule worth it?  IMO, no; it's too trivial, and I don't care if someone names their test file something different; I don't see a ton of that in the codebase and it hasn't come up in arguments.  So this isn't a fraught issue begging for a solution, and I think we just shouldn't write anything in the style guide.  Status quo is OK for me.

I bow to whatever the consensus of the group is.

PK

[Michael Ershov]

I don’t have a strong opinion about naming files, but I think we already have a pretty serious problem with finding a way to run a specific test.

If we imagine that I modified some random file with a _test.cc suffix and now I want to compile and run it, it could be in unit_tests, browser_tests, components_unittests, chromeos_unittests, lacros_chrome_browsertests and probably several other binaries.
Could we introduce a way to figure this out?
Maybe enforce a comment about this in the header of test files?
Today I have to read BUILD.gn files, sometimes several layers of them and I think it’s not convenient and not simple, especially for less experienced contributors.

--

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 view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAAHOzFCs%2BzMY-V%2B_7tHSf89kTJEEay7XPR4o%2BbvE9ZnzqSuu2Q%40mail.gmail.com.

[Peter Kasting]

On Mon, Jul 5, 2021 at 5:11 AM Michael Ershov <mie...@google.com> wrote:

I don’t have a strong opinion about naming files, but I think we already have a pretty serious problem with finding a way to run a specific test.

If we imagine that I modified some random file with a _test.cc suffix and now I want to compile and run it, it could be in unit_tests, browser_tests, components_unittests, chromeos_unittests, lacros_chrome_browsertests and probably several other binaries.
Could we introduce a way to figure this out?

gn/ninja can do this, but i forget how.

PK 

[Leszek Swirski]

There's gn refs, e.g.

$ gn refs out/Release services/device/wake_lock/wake_lock_unittest.cc --all          
//:gn_all
//services:services_unittests
//services/device:tests

--

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 view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAAHOzFAcywqNkdWPLGOcE8sTgeBcR%3DPsP91kWGdaJDOOC%2BOLFQ%40mail.gmail.com.

[Michael Ershov]

That's great, thanks for the tip!

Michael Ershov

Software Engineer

mie...@google.com

Google Germany GmbH

Erika-Mann-Straße 33

80636 München

Geschäftsführer: Paul Manicle, Halimah DeLaine Prado

Registergericht und -nummer: Hamburg, HRB 86891

Sitz der Gesellschaft: Hamburg

Diese E-Mail ist vertraulich. Falls Sie diese fälschlicherweise erhalten haben sollten, leiten Sie diese bitte nicht an jemand anderes weiter, löschen Sie alle Kopien und Anhänge davon und lassen Sie mich bitte wissen, dass die E-Mail an die falsche Person gesendet wurde. 

     

This e-mail is confidential. If you received this communication by mistake, please don't forward it to anyone else, please erase all copies and attachments, and please let me know that it has gone to the wrong person.

[Joe Mason]

There's also a tools/autotest.py wrapper that takes a _*test.cc filename, finds the right test suite for it, and compiles and runs that test suite, passing through gtest params.

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

[Jeremy Roman]

Adding --type=executable is great because it will give you exactly the (test) binaries and nothing else.

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

[Dominic Farolino]

Do any others have strong opinions on the naming of tests? Personally I think keeping _browsertest.cc and _unittest.cc suffixes are a good idea, and I'm in favor of documenting this in the style guide.

[Jeremy Roman]

I think my bias would be "be consistent with existing code in the area". Blink tends to use _test.cc for historical reasons; content tends to use _browsertest.cc and _unittest.cc for good local reasons.

[dan...@chromium.org]

On Mon, Jul 5, 2021 at 2:35 PM Jeremy Roman <jbr...@chromium.org> wrote:

I think my bias would be "be consistent with existing code in the area". Blink tends to use _test.cc for historical reasons; content tends to use _browsertest.cc and _unittest.cc for good local reasons.

I think blink has been moving toward being consistent with the rest of chromium: https://source.chromium.org/search?ss=chromium%2Fchromium%2Fsrc&q=f:unittest%20f:blink

[Sylvain Defresne]

Chrome on iOS also uses _egtest.mm for tests of the whole app (kinda like browser tests, but they can start/stop/relaunch the app) and _inttest.mm for integration tests (between unit tests and whole app tests).

-- Sylvain

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

[Jeremy Roman]

91 results: https://source.chromium.org/search?q=f:_unittest%5C.cc$%20f:blink%2Frenderer&ss=chromium%2Fchromium%2Fsrc

1448 results: https://source.chromium.org/search?q=f:_test%5C.cc$%20f:blink%2Frenderer&sq=&ss=chromium%2Fchromium%2Fsrc

If it is moving toward that direction, it's doing so rather slowly.

[K. Moon]

To answer the question about, "What does google3 do," it essentially boils down to the question of what Bazel does. Bazel allows test targets to be tagged with various labels; "size" is commonly used for this purpose (unit tests typically are "small", other kinds of tests are not):

https://docs.bazel.build/versions/main/be/common-definitions.html#common-attributes-tests

If the categories are more complicated, you can use "tags" to assign arbitrary labels to a target. (I've used this in the past to mark targets that should only be executed by a certain kind of test runner.)

GN/Ninja doesn't have any of these features, AFAIK, so I'm not sure if it makes sense to import google3 practices here.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CACuR13eEp2NCyzynTnxzZ3p88yFZCRRy1pOo2_19tJ2a0%2BZ4-A%40mail.gmail.com.

[K. Moon]

Another difference is that test binaries typically are defined close to the test source files, rather than being scattered over the entire repo, and collected into just a handful of test binaries.

[Peter Kasting]

How I read this thread so far:

• Clear consensus to keep naming files the way we're currently doing (_unittest, _browsertest, etc.)
• Positive sentiment for modifying the Chromium style guide to say to do that, since it contradicts the Google style guide.  Less clear to me that this is at "consensus" stage (or maybe I'm being overcautious?)

Are there any other opinions, especially on the second bullet?

PK

[dan...@chromium.org]

On Thu, Jul 8, 2021 at 8:46 AM 'Peter Kasting' via cxx <c...@chromium.org> wrote:

How I read this thread so far:

• Clear consensus to keep naming files the way we're currently doing (_unittest, _browsertest, etc.)
• Positive sentiment for modifying the Chromium style guide to say to do that, since it contradicts the Google style guide.  Less clear to me that this is at "consensus" stage (or maybe I'm being overcautious?)

While remembering the contents of the style guide is reasonable goal, I feel the primary purpose of the guide is to avoid disagreements in code review. When a disagreement would happen, there's somewhere to point to such that it is resolved immediately.

Since the Google guide differs it becomes worse right now as there's actually somewhere to point that increases the disagreement (guide vs code) so adding to our guide would help resolve this.

 

Are there any other opinions, especially on the second bullet?

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 view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAAHOzFA%3DK3i-bq2-OEewQRkiYMP%3D9PKaSkjoLbJPEt123v3tJQ%40mail.gmail.com.