Re: Should We Remove Unnecessary Underscores in TestSuiteName and TestName in Chromium Tests?

[Alexei Svitkine]

Thanks for starting the email.

I guess I raised concerns because:

1. The change hurts readability since many tests with underscores have explicitly been named that way to make their names more readable, e.g.:

TEST_F(FieldTrialTest, SetForcedChangeDefault_Default) {

TEST_F(FieldTrialTest, SetForcedChangeDefault_NonDefault) {

Which would become:

TEST_F(FieldTrialTest, SetForcedChangeDefaultDefault) {

TEST_F(FieldTrialTest, SetForcedChangeDefaultNonDefault) {

2. In google3, which also uses GoogleTest, this naming convention is also very common and there's no centralized effort being done to change things. So it seems unlikely to break.

3. And the GoogleTest docs specifically mentioning that the issue with the underscore in the middle case is to prevent collisions of the form:

TEST(Time, Flies_Like_An_Arrow) { ... }
TEST(Time_Flies, Like_An_Arrow) { ... }

Which IMHO are extremely unlikely and would be obvious link errors.

So the value proposition seems minimal at the cost of readability to affected tests and code churn / effort.

-Alexei

On Fri, Nov 22, 2024 at 7:56 AM Ho Cheung <hoch...@chromium.org> wrote:

Hello everyone,

Inspired by a related issue in PDFium [1], I have initiated a similar issue in Chromium, details of which can be found in [2]. Currently, Helmut Januschka and I are actively working on this.

The primary goal of this issue is to clean up unnecessary underscores in the TestSuiteName and TestName of tests written using the GoogleTest framework in the Chromium codebase (excluding special prefixes like MAYBE_ and DISABLED_).

However, during the cleanup of the //base directory, Alexei Svitkine raised some concerns and suggested further discussion on this topic. Details of his feedback can be found in [3].

From my perspective, removing unnecessary underscores in TestSuiteName and TestName is reasonable. The GoogleTest documentation supports this to some extent, even though the examples provided are limited. For reference, please see [4]:

```
So for simplicity, we just ask the users to avoid _ in TestSuiteName and TestName. The rule is more constraining than necessary, but it's simple and easy to remember. It also gives GoogleTest some wiggle room in case its implementation needs to change in the future.
```

Given the current lack of consensus, I’d like to seek input from the Chromium Slack: Should we adopt the practice of cleaning up unnecessary underscores in TestSuiteName and TestName within the Chromium project?

[1] https://issues.chromium.org/issues/42270790
[2] https://issues.chromium.org/issues/377144451
[3] https://chromium-review.googlesource.com/c/chromium/src/+/6034789?tab=comments
[4] https://github.com/google/googletest/blob/main/docs/faq.md#why-should-test-suite-names-and-test-names-not-contain-underscore

[Christoph Schwering]

+1 for keeping underscores because they improve readability a lot IMO.

--
--
Chromium Developers mailing list: chromi...@chromium.org
View archives, change email options, or unsubscribe:
http://groups.google.com/a/chromium.org/group/chromium-dev
---
You received this message because you are subscribed to the Google Groups "Chromium-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to chromium-dev...@chromium.org.
To view this discussion visit https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CAKFU2SAR_wqcKo2uGuJGW9tD9X9qi-aS%2BF%2BThiL6BcJW9RZSjA%40mail.gmail.com.

--

Google Germany GmbH

Erika-Mann-Straße 33

80636 München

Geschäftsführer: Paul Manicle, Liana Sebastian

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 email 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.

[Peter Kasting]

I think Alexei's concerns are reasonable. For cases where we have blocks of camel case text separated by underscores, I would leave them.

I also think there are likely tests that use underscores that need not do so. IMO it is fractionally nicer to be consistent with other chrome code and the gtest docs by changing those. I haven't looked so I don't have examples, and I might be wrong. 

My conclusion would be that we could use tooling to identify places to look at, but we couldn't just blindly auto-change them, and we wouldn't want to force the codebase to be clean under a clang-tidy check for this. 

PK

--

[Sylvain Defresne]

Also +1 from me for keeping underscores in test names, they help a lot with readability, e.g. when testing different behaviour (like failure mode) of a single method in separate test cases.

-- Sylvain

To view this discussion visit https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CAAHOzFCs%3D-BgKdWf79H1AQR-hdsUh71FGAaMrhwyaDVyQcBc-Q%40mail.gmail.com.

[Ho Cheung]

[Ho Cheung]

Hi everyone,

Thank you for the thoughtful responses and insights on this topic. I appreciate the time you've taken to consider the implications of this working.

From the discussion so far, I understand that there are differing perspectives:

1. Concerns about readability

Alexei and others have pointed out that underscores can significantly enhance the readability of test names, especially in cases like:

```
TEST_F(FieldTrialTest, SetForcedChangeDefault_Default)  
TEST_F(FieldTrialTest, SetForcedChangeDefault_NonDefault)  
```

Removing underscores would make such names harder to read and understand at a glance, e.g.:

```
TEST_F(FieldTrialTest, SetForcedChangeDefaultDefault)  
TEST_F(FieldTrialTest, SetForcedChangeDefaultNonDefault)
```

2. GoogleTest documentation and consistency

While GoogleTest suggests avoiding underscores to prevent rare name collisions (e.g., TEST(Time, Flies_Like_An_Arrow) vs. TEST(Time_Flies, Like_An_Arrow)), Alexei and others noted that such cases are rare and would likely result in obvious link errors. Furthermore, it seems there is no push within Google3 to enforce this rule, even though the naming style with underscores is common there.

3. Use tools for identification but avoid making blind changes

Use tooling to identify test names where underscores might not be necessary, but avoid blindly making automated changes to all test names.


I will continue gathering feedback and revisit the changes made earlier at an appropriate time (perhaps in a week or a few days).

Looking forward to your thoughts.

Best regards.

-Ho Cheung

Peter Kasting <pkas...@chromium.org> 於 2024年11月23日週六 上午2:38寫道：

[Ho Cheung]

I'll add Adam's comments here.

```

Personally, seeing underscores in test names has always annoyed me.

```

Ho Cheung <hoch...@chromium.org> 於 2024年11月23日週六 上午9:37寫道：

[Alexei Svitkine]

+1, that SGTM too.

On Fri, Dec 6, 2024 at 10:12 AM Ryan Tarpine <rtar...@google.com> wrote:

It seems to me that forbidding underscores in TestSuiteName, but allowing them in TestName, would prevent the generated-class-name-clashing issue while permitting the vast majority of existing underscore usage in Chromium (so minimal churn to fix).

Underscore usage is widespread enough that I really can't imagine GoogleTest breaking that in a future update ¯\_(ツ)_/¯

Ryan

To view this discussion visit https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CAGgvpQ%3Dg6F1r5-fGeLyJVNhq35jrSzo1Z08MM_XHoQ-PBdidOA%40mail.gmail.com.

[Wez]

Thanks for taking the time to raise this, draft CLs, and discuss it here, Ho Cheung. :)

On Fri, 6 Dec 2024 at 16:32, Ho Cheung <hoch...@chromium.org> wrote:

As of now, there are no more problems or issues with this topic, and all previously merged patches have been reverted.

[Ryan Tarpine]

It seems to me that forbidding underscores in TestSuiteName, but allowing them in TestName, would prevent the generated-class-name-clashing issue while permitting the vast majority of existing underscore usage in Chromium (so minimal churn to fix).

Underscore usage is widespread enough that I really can't imagine GoogleTest breaking that in a future update ¯\_(ツ)_/¯

Ryan

To view this discussion visit https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CAGgvpQ%3Dg6F1r5-fGeLyJVNhq35jrSzo1Z08MM_XHoQ-PBdidOA%40mail.gmail.com.

[Ho Cheung]

As of now, there are no more problems or issues with this topic, and all previously merged patches have been reverted.

Alexei Svitkine <asvi...@chromium.org> 於 2024年12月6日 週五 23:29 寫道：

[Mark Gunn]

Is there any videos I can watch to learn this stuff.  ,?? I'm so confused on the header stuff I click one thing and it doesn't do anything I need help

To view this discussion visit https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CALekkJcjWzMOGiFXQgDxPC3qNC_inw3Y8j8aGha0WimoDyhi9A%40mail.gmail.com.