Skip to content

Conversation

@KangarooKoala
Copy link
Contributor

@KangarooKoala KangarooKoala commented Dec 10, 2025

Consistently using (gtest|catch2|java|python)_tests makes it easier to remember how to run tests.

Everything is now [project]-[lang]-test, except for wpiutil catch2 and gtest cpp tests
@KangarooKoala KangarooKoala requested review from a team and PeterJohnson as code owners December 10, 2025 21:04
@github-actions github-actions bot added component: ntcore NetworkTables library component: wpiutil Utility library component: wpilibc WPILib C++ component: hal Hardware Abstraction Layer component: wpimath Math library component: wpinet Networking library component: apriltag AprilTag library component: examples component: wpiunits Java units library component: epilogue Annotation-based logging library 2027 2027 target labels Dec 10, 2025
Copy link
Member

@auscompgeek auscompgeek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some different points for discussion. Probably need to decide what to actually do out of these first 😄

@pjreiniger
Copy link
Contributor

Who knows if/ when non wpiutil tests will start using catch2, but maybe you plan for that now, i.e java_rests, python_tests, gtest_tests, catch2_tests (if applicable)

@KangarooKoala
Copy link
Contributor Author

KangarooKoala commented Dec 13, 2025

While we're at it, any preference on underscore versus hyphen? And do we want to use "test" or "tests"?

@KangarooKoala KangarooKoala requested review from a team as code owners December 13, 2025 20:28
@github-actions github-actions bot added component: cscore CameraServer library component: wpilibj WPILib Java labels Dec 13, 2025
@pjreiniger
Copy link
Contributor

While we're at it, any preference on underscore versus hyphen? And do we want to use "test" or "tests"?

I don't super care as long as its consistent. I'd probably lean towards plural, and I guess underscore feels more natural for a "programming language" feel

@Gold856
Copy link
Member

Gold856 commented Dec 14, 2025

gtest_tests/catch2_tests is kinda tricky. In theory, the distinction wouldn't be necessary as we translate a given subproject's tests in one go, but the presence of thirdparty tests makes this a little bit more complicated. Taking a look at those, it seems like the only one that we haven't really written manually is LLVM. Realistically, everything else is our own code that we can translate, but LLVM being here kinda sucks because it uses gtest, and translating it to Catch2 will make rebases annoying, and special casing as its own target it will make it easy to miss when building locally. I think I prefer a separate target anyways because it should still get run by CI and it's relatively rare to modify LLVM and require rerunning those tests.

@auscompgeek
Copy link
Member

The BUILD style guide seems to suggest using underscores in target names. https://bazel.build/build/style-guide


cc_test(
name = "apriltag-cpp-test",
name = "gtest_tests",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this tests the code built by the apriltag rule I recommend naming it apriltag_test or apriltag_tests.

This is convention used internally at Google. When you are making changes to code built by a C++ library target named "foo", and you want to run the tests for that target, the target is generally named "foo_test", which is easy to remember. The same convention is used for Python and Go.

(you can, of course, run all of the tests in the package via :all, but for some packages that can be slow, so people would iterate by running the individual target, and run all of the tests before committing)

If you use bash or zsh and setup autocompletion then you can type "bazel test //apriltag:foo_" and press tab and it will auto-complete (so I don't have to remember whether the build target ends in "s").

I personally don't see the benefit to encoding the test framework in the build target. Would it be common for a single build target to have multiple test targets, each using different test frameworks?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Merry Christmas!

Since this tests the code built by the apriltag rule I recommend naming it apriltag_test or apriltag_tests.

apriltag_test or apriltag_tests wouldn't work, since we need to include some information about the language to disambiguate the C++, Java, and Python test targets.

When you are making changes to code built by a C++ library target named "foo", and you want to run the tests for that target, the target is generally named "foo_test", which is easy to remember.

In allwpilib, there's a one-to-one correspondence between packages and library targets, so there's no point in including the library target name in the test name (because it's identical to the package name). See also this thread for previous discussion on dropping the package name.

Would it be common for a single build target to have multiple test targets, each using different test frameworks?

Right now, wpiutil has both, so there needs to be some way to disambiguate the two. That's probably going to be the only case, though, so it maybe it'd be better to use cpp_tests for everything except wpiutil? CC @pjreiniger since you recommended switching to gtest_tests and catch2_tests.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

2027 2027 target component: apriltag AprilTag library component: cscore CameraServer library component: epilogue Annotation-based logging library component: examples component: hal Hardware Abstraction Layer component: ntcore NetworkTables library component: wpilibc WPILib C++ component: wpilibj WPILib Java component: wpimath Math library component: wpinet Networking library component: wpiunits Java units library component: wpiutil Utility library

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants