Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Underscores shouldn't be used in googletest test names #2637

Closed
tiagoshibata opened this issue Dec 12, 2019 · 3 comments
Closed

Underscores shouldn't be used in googletest test names #2637

tiagoshibata opened this issue Dec 12, 2019 · 3 comments
Assignees

Comments

@tiagoshibata
Copy link
Contributor

Describe the bug
Use of underscores in test case names (e.g. TEST(DateTimeTransformer, Past_1976_Nov_17__12_27_04)) in googletest is not recommended, since it has a few gotchas (https://github.com/google/googletest/blob/master/googletest/docs/faq.md#why-should-test-suite-names-and-test-names-not-contain-underscore). Furthermore, some tests (such as TEST(DateTimeTransformer, Past_1976_Nov_17__12_27_04)) have double underscores, which are reserved to be used by the compiler and standard library. It's not a big issue and unlikely to break, but it's not standards conforming and, quoting googletest's docs:

If you violate the rule, there may not be immediate consequences, but your test may (just may) break with a new compiler (or a new version of the compiler you are using) or with a new version of googletest. Therefore it's best to follow the rule.

Urgency
N/A

System information
N/A

To Reproduce
Files such as onnxruntime\test\automl_ops\datetimetransformer_test.cc have a bunch of underscores in test names.

@snnn
Copy link
Member

snnn commented Dec 12, 2019

@yuslepukhin Please take a look

@skottmckay
Copy link
Contributor

If I understand the FAQ correctly you can use a single underscore as long as it's not the first character in the test name and followed by an upper case character. I don't believe we have any invalid usage of a single underscore based on that.

The usage of a double underscore should be easily fixed.

@tiagoshibata
Copy link
Contributor Author

If I understand the FAQ correctly you can use a single underscore as long as it's not the first character in the test name and followed by an upper case character

It's more restrictive. Single underscores:

  • Can't be the first character in a test suite name if followed by an upper case character
  • Can't be the first character in a test name
  • Can't be the last character of a test suite name or test name

And it's tricky to use since _ is used as separator between the test suite name and test name (so TEST(Time, Flies_Like_An_Arrow) and TEST(Time_Flies, Like_An_Arrow) generate the same class name and a compilation or linking error). Since these rules are awkward, their recommendation is to simply not use _.

I opened the issue to raise awareness of this (on our porting of WinML we're following their recommendation). Fixing the use of double underscores and keeping the safe usages of single underscores sounds fine to me, just be aware of the possible duplicate class name issue :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants