@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.
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.)
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.
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.
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