Requiring global unique names for unit tests in Stan Math

@alashworth has raised an issue that we have non-unique names for the unit test. This prevents any sort of work on jumbo (unity) builds of the unit tests. Having those would have a large benefit for test compile time, so this is very relevant.

To clarify, if we have TEST(MathMatrix, inverse_exception) that same pair of names should not be used in any other test case in any other files in our codebase.

I imagine if I go and fix that in the current codebase that would not be an issue to anyone since its just a rename PR.

But how do you feel if we add this to be a requirement on PRs?

We can add a check to runChecks.py that enables the developers to double check this on their own and this will also be run on the Jenkins tests so no PRs with non-unique names could be merged.

I really dont want to add such a restriction on my own. It would definitely be added to the developer doc.

Tagging some developers that worked on Math recently.

@syclik @Bob_Carpenter @Stevo15025 @wds15 @tadej @andrjohns @yizhang @charlesm93 @seantalts

2 Likes

Seems fine to me - I’m also assuming that at some point we’ll be switching to the cmake builds anyway so it will also complain if duplicate names are found. Seems like a small price to pay for unity builds for tests which I’m assuming will speed things up a fair bit.

Yes! If you can get anything automated, we can include it.

The way we’ve handled these things in the past: start with automated warnings. Then we change the existing implementation to have 0 warnings. At that point, when it’s clear that the policy is good, that it’s enforceable, and the math library has no warnings, we can start enforcing it for all new contributions

One thing we need to do is to provide better guidance on names of tests. We’ve gone through some changes since the start and we haven’t had any guidance, which is why it’s what it is. (Also, it wasn’t so important as long as it was unique within the translation unit, but this is the fundamental difference that we’re now thinking about.)

1 Like

I’m always in favour of simplifying maintenance. As long as that runChecks.py check is in place, it should be easy enough for developers to keep up.

It might also be worth adding another checkbox to the math PR template, so that developers know to check it before submitting

1 Like

How strict are we about the naming conventions?

One thing @yizhang and I started using are test fixtures in order to share data across multiple unit tests. This avoids duplicate code while keeping unit tests elementary; and moreover greatly simplifies tests for functions which have multiple implementations. For example, tests for our two algebraic solvers. We could do the same with our ODE solvers.

The limitation with fixtures is that they place restrictions on the name of the macro, so you end up with tests named TEST_F(algebra_solver_simple_eq, powell) and TEST_F(algebra_solver_simple_eq, newton). That said, this doesn’t strike me as an issue.

It’s been used long before newton PR.

Yes, you’re right, I see we have it for the CVODES tests. Ok, well that’s a non-issue then.

We’ve ignored one of the main Google Test naming rules from early on: https://github.com/google/googletest/blob/master/googletest/docs/faq.md#why-should-test-suite-names-and-test-names-not-contain-underscore

We did that because practically, it didn’t have an effect. The naming clash that’s described on the FAQ holds within a translation unit. Since we had small tests, this didn’t matter. Now that we’re trying to change that, it does.

So how strict? Right now, there’s no enforcement on naming. It’s probably better to make tests after the function so no one is confused, but if you wrote a test and it had underscores or used camel case, it didn’t matter.

After we start down this road, it should and will matter and we’ll have to be strict in order to avoid conflicts since the goal is to have everything in a single translation unit.

Yes. I will add on to this we will also need to be careful with use of the EXPECT_DEATH macro and test naming regarding that as well.

I agree that if it can be automated, it’d be good to test.

A PR for this is finally live: https://github.com/stan-dev/math/pull/1422

2 Likes

This rule is now in full effect since yesterday.

In order to check locally just use make test-math-dependencies. The check scripts runs in Python so that we are not dependent on any gnu tools or anything.

It will output something like:

Tests or test fixtures with non-unqiue names found in test/unit:

(MathMatrix, matrix_exp_pade_1x1) in files:
	test/unit/math/rev/mat/fun/matrix_exp_pade_test.cpp
	test/unit/math/fwd/mat/fun/matrix_exp_pade_test.cpp
	test/unit/math/prim/mat/fun/matrix_exp_pade_test.cpp

(MathMatrix, matrix_exp_multiply_exception) in files:
	test/unit/math/rev/mat/fun/matrix_exp_multiply_test.cpp
	test/unit/math/prim/mat/fun/matrix_exp_multiply_test.cpp

(ErrorHandlingScalar, CheckNonnegative_nan) in files:
	test/unit/math/prim/scal/err/check_nonnegative_test.cpp
	test/unit/math/prim/arr/err/check_nonnegative_test.cpp

(MathMeta, value_type) in files:
	test/unit/math/rev/scal/meta/child_type_test.cpp
	test/unit/math/fwd/scal/meta/child_type_test.cpp
	test/unit/math/prim/scal/meta/child_type_test.cpp
	test/unit/math/prim/arr/meta/value_type_test.cpp
	test/unit/math/mix/scal/meta/child_type_test.cpp

Example of a failed Jenkins run is here:

https://jenkins.mc-stan.org/blue/organizations/jenkins/Math%20Pipeline/detail/PR-1422/1/pipeline/

I suppose this should also be written on a wiki somwhere? Any ideas where?

2 Likes

Awesome. Maybe the CONTRIBUTING.md?

1 Like

Another good spot might be here: https://github.com/stan-dev/math/wiki/Developer-Doc#pull-requests