Add restriction for unique test names by rok-cesnovar · Pull Request #1422 · stan-dev/math

rok-cesnovar · 2019-10-25T17:22:08Z

Summary

This PR adds the restriction of having unique names for TEST() and TEST_F() across the entire test/unit folder. It does so by adding a check to runChecks.py that looks for duplicates in all test/unit/*_test.cpp files.

Developers can verify they have not introduced non-unique names by running python runChecks.py.

The first commit will be the python script only. That way we can see the errors on Jenkins to verify that it works OK there. Then I will merge the cleanup branch that fixes all the errors.

Tests

/

Side Effects

/

Checklist

Math issue Suggestion: enforce unique GoogleTest names #1387
Copyright holder: Rok Češnovar
the basic tests are passing
the code is written in idiomatic C++ and changes are documented in the doxygen
the new changes are tested

…e-test-name

rok-cesnovar · 2019-10-25T17:43:17Z

You can check how the error look like on Jenkins https://jenkins.mc-stan.org/blue/organizations/jenkins/Math%20Pipeline/detail/PR-1422/1/pipeline/

An example:

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

…gs/RELEASE_500/final)

rok-cesnovar · 2019-10-25T18:01:52Z

This one is going to be a pain to review because this touches A LOT of files
So to help out here are the changes that are not just renaming tests:

39395b2 removes unused tests that were behind #if 0
8ecff0a deletes a test file that was never used (fma.hpp cant be triggered) and adds non-duplicates tests to fma_test.cpp
c80b13e removes duplicate tests

The rest just rename tests. I tried to make some of the generic labels like MathMatrix more specific with at least MathMatrixRevScal or something along those lines.

rok-cesnovar · 2019-10-25T18:03:12Z

Also this will probably cause merge issues with @bob-carpenter's efforts on the test framework. I am also happy to hold off on this PR until that gets merged.

# Conflicts: # test/unit/math/prim/mat/fun/log_determinant_ldlt_test.cpp # test/unit/math/prim/mat/fun/matrix_exp_pade_test.cpp # test/unit/math/prim/mat/fun/matrix_exp_test.cpp

# Conflicts: # test/unit/math/prim/mat/fun/matrix_exp_multiply_test.cpp # test/unit/math/prim/mat/fun/scale_matrix_exp_multiply_test.cpp # test/unit/math/rev/mat/fun/scale_matrix_exp_multiply_test.cpp

t4c1

This is mostly fine, just some nitpicking from me.

runChecks.py

test/unit/math/fwd/arr/meta/partials_return_type_test.cpp

test/unit/math/prim/mat/fun/cov_matrix_transform_test.cpp

test/unit/math/prim/mat/fun/log_determinant_ldlt_test.cpp

test/unit/math/rev/mat/functor/integrate_ode_adams_rev_test.cpp

test/unit/math/rev/mat/functor/integrate_ode_bdf_rev_test.cpp

Co-Authored-By: Tadej Ciglarič <tadej.c@gmail.com>

rok-cesnovar · 2019-11-25T14:26:14Z

Thanks for the python script comments! I think I addressed everything.

t4c1

Looks good.

rok-cesnovar · 2019-11-25T15:25:01Z

There were some additional non-unique tests that were found after the merge. The push dismissed the review :/

# Conflicts: # test/unit/math/prim/mat/fun/inverse_test.cpp

t4c1

Looks good.

stan-buildbot · 2019-11-27T20:32:49Z

(stat_comp_benchmarks/benchmarks/gp_pois_regr/gp_pois_regr.stan, 1.01)
(stat_comp_benchmarks/benchmarks/low_dim_corr_gauss/low_dim_corr_gauss.stan, 1.01)
(stat_comp_benchmarks/benchmarks/irt_2pl/irt_2pl.stan, 0.99)
(stat_comp_benchmarks/benchmarks/pkpd/one_comp_mm_elim_abs.stan, 0.96)
(stat_comp_benchmarks/benchmarks/eight_schools/eight_schools.stan, 1.04)
(stat_comp_benchmarks/benchmarks/gp_regr/gp_regr.stan, 0.99)
(stat_comp_benchmarks/benchmarks/arK/arK.stan, 1.0)
(performance.compilation, 1.03)
(stat_comp_benchmarks/benchmarks/low_dim_gauss_mix_collapse/low_dim_gauss_mix_collapse.stan, 1.0)
(stat_comp_benchmarks/benchmarks/low_dim_gauss_mix/low_dim_gauss_mix.stan, 1.0)
(stat_comp_benchmarks/benchmarks/sir/sir.stan, 1.01)
(stat_comp_benchmarks/benchmarks/pkpd/sim_one_comp_mm_elim_abs.stan, 1.02)
(stat_comp_benchmarks/benchmarks/garch/garch.stan, 1.01)
(stat_comp_benchmarks/benchmarks/gp_regr/gen_gp_data.stan, 0.98)
(stat_comp_benchmarks/benchmarks/arma/arma.stan, 1.0)
Result: 1.00333709785
Commit hash: 8795ab4

syclik

+1 from me.

syclik · 2019-11-28T05:18:12Z

@rok-cesnovar, somehow the tests haven't passed. Can you kick this off again?

Also, with this PR, has this been integrated into the continuous integration?

rok-cesnovar · 2019-11-28T07:54:21Z

Yeah, some upstream tests timeouted or something weird. Restarted now.

These checks are part of runChecks.py that is run with make test-math-dependencies (https://github.com/stan-dev/math/blob/develop/Jenkinsfile#L131). Those are run in the first phases of a Jenkins tests.

An example of what the check does on develop (develop when I started this PR to be exact) is given here:
https://jenkins.mc-stan.org/blue/organizations/jenkins/Math%20Pipeline/detail/PR-1422/1/pipeline/

rok-cesnovar · 2019-11-28T07:57:06Z

@serban-nicusor-toptal sorry to bother you again, this seems like a weird failure. It might be some issue I caused on this PR, but I am seeing no errors printed: https://jenkins.mc-stan.org/blue/organizations/jenkins/Math%20Pipeline/detail/PR-1422/33/pipeline

Thanks!

serban-nicusor-toptal · 2019-11-28T08:03:49Z

Fixed, was trying to run on the new windows machine. It was looking for an agent and it picked the new one because I didn't restrict it.

stan-buildbot · 2019-11-28T15:12:25Z

(stat_comp_benchmarks/benchmarks/gp_pois_regr/gp_pois_regr.stan, 1.0)
(stat_comp_benchmarks/benchmarks/low_dim_corr_gauss/low_dim_corr_gauss.stan, 1.0)
(stat_comp_benchmarks/benchmarks/irt_2pl/irt_2pl.stan, 1.0)
(stat_comp_benchmarks/benchmarks/pkpd/one_comp_mm_elim_abs.stan, 1.06)
(stat_comp_benchmarks/benchmarks/eight_schools/eight_schools.stan, 1.05)
(stat_comp_benchmarks/benchmarks/gp_regr/gp_regr.stan, 0.99)
(stat_comp_benchmarks/benchmarks/arK/arK.stan, 0.99)
(performance.compilation, 1.03)
(stat_comp_benchmarks/benchmarks/low_dim_gauss_mix_collapse/low_dim_gauss_mix_collapse.stan, 1.01)
(stat_comp_benchmarks/benchmarks/low_dim_gauss_mix/low_dim_gauss_mix.stan, 1.01)
(stat_comp_benchmarks/benchmarks/sir/sir.stan, 1.02)
(stat_comp_benchmarks/benchmarks/pkpd/sim_one_comp_mm_elim_abs.stan, 1.06)
(stat_comp_benchmarks/benchmarks/garch/garch.stan, 1.0)
(stat_comp_benchmarks/benchmarks/gp_regr/gen_gp_data.stan, 1.01)
(stat_comp_benchmarks/benchmarks/arma/arma.stan, 1.0)
Result: 1.01668932471
Commit hash: 8795ab4

rok-cesnovar added 23 commits October 25, 2019 12:46

agrad changed to *Functor

c5e7e2b

fma, odegrad

8e0d574

integrate_prim and integrate_rev tests

0668dfc

integrate_ode_*_rev

be8a569

require

9f31610

duplicated tests

c80b13e

prob

0586622

metaTraits pt1

71e4377

metaTraits pt2

ba356da

metaTraits end

b1fedf0

mathmeta

16b99be

MathMatrix

186f3c9

math fun

929f0f8

fma tests duplicates

8ecff0a

prec2

3132a36

various

cc40eda

check_varis_on_stack, mix tests with Fwd in name

668e3d0

cleanup residual

591c3d5

remove unused tests

39395b2

varis on stack

ccbc944

final cleanup

9093179

finished script

618aca1

Merge branch 'cleanup-duplicated-test-names' into feature/check-uniqu…

bd4afee

…e-test-name

yashikno and others added 3 commits October 25, 2019 17:44

Merge commit '6723f81778f3ba939a95908c70d1514bfd322713' into HEAD

2f29274

[Jenkins] auto-formatting by clang-format version 5.0.0-3~16.04.1 (ta…

b711839

…gs/RELEASE_500/final)

newline

1aed434

missing header

b381416

rok-cesnovar added 3 commits November 25, 2019 12:25

Merge branch 'develop' into feature/check-unique-test-name

9f36bc3

# Conflicts: # test/unit/math/prim/mat/fun/log_determinant_ldlt_test.cpp # test/unit/math/prim/mat/fun/matrix_exp_pade_test.cpp # test/unit/math/prim/mat/fun/matrix_exp_test.cpp

Merge branch 'develop' into feature/check-unique-test-name

bd769e5

# Conflicts: # test/unit/math/prim/mat/fun/matrix_exp_multiply_test.cpp # test/unit/math/prim/mat/fun/scale_matrix_exp_multiply_test.cpp # test/unit/math/rev/mat/fun/scale_matrix_exp_multiply_test.cpp

fix duplicate after merge

f329ebf

t4c1 requested changes Nov 25, 2019

View reviewed changes

rok-cesnovar and others added 5 commits November 25, 2019 14:03

Update runChecks.py

8d0e0be

Co-Authored-By: Tadej Ciglarič <tadej.c@gmail.com>

Update runChecks.py

7caf379

Co-Authored-By: Tadej Ciglarič <tadej.c@gmail.com>

Update runChecks.py

29a615e

Co-Authored-By: Tadej Ciglarič <tadej.c@gmail.com>

Update test/unit/math/fwd/arr/meta/partials_return_type_test.cpp

0f9089d

Co-Authored-By: Tadej Ciglarič <tadej.c@gmail.com>

Update test/unit/math/prim/mat/fun/log_determinant_ldlt_test.cpp

0c7975d

Co-Authored-By: Tadej Ciglarič <tadej.c@gmail.com>

rok-cesnovar requested a review from t4c1 November 25, 2019 13:27

improve regex

9af481c

t4c1 previously approved these changes Nov 25, 2019

View reviewed changes

fix more non-unique tests

98b3c69

rok-cesnovar dismissed t4c1’s stale review via 98b3c69 November 25, 2019 15:22

rok-cesnovar added 3 commits November 25, 2019 17:02

change the TEST_F name

478ca42

Merge branch 'develop' into feature/check-unique-test-name

cf47fd3

# Conflicts: # test/unit/math/prim/mat/fun/inverse_test.cpp

test fixture name fix

8795ab4

t4c1 approved these changes Nov 27, 2019

View reviewed changes

syclik approved these changes Nov 28, 2019

View reviewed changes

rok-cesnovar merged commit 601cba9 into develop Nov 29, 2019

rok-cesnovar deleted the feature/check-unique-test-name branch November 29, 2019 18:11

rok-cesnovar mentioned this pull request Dec 31, 2019

Flatten /err tests #1562

Merged

5 tasks

Uh oh!

Conversation

rok-cesnovar commented Oct 25, 2019

Summary

Tests

Side Effects

Checklist

Uh oh!

rok-cesnovar commented Oct 25, 2019

Uh oh!

rok-cesnovar commented Oct 25, 2019

Uh oh!

rok-cesnovar commented Oct 25, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

t4c1 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rok-cesnovar commented Nov 25, 2019

Uh oh!

t4c1 left a comment

Choose a reason for hiding this comment

Uh oh!

rok-cesnovar commented Nov 25, 2019

Uh oh!

t4c1 left a comment

Choose a reason for hiding this comment

Uh oh!

stan-buildbot commented Nov 27, 2019

Uh oh!

syclik left a comment

Choose a reason for hiding this comment

Uh oh!

syclik commented Nov 28, 2019

Uh oh!

rok-cesnovar commented Nov 28, 2019

Uh oh!

rok-cesnovar commented Nov 28, 2019

Uh oh!

serban-nicusor-toptal commented Nov 28, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

stan-buildbot commented Nov 28, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

rok-cesnovar commented Oct 25, 2019 •

edited

Loading

serban-nicusor-toptal commented Nov 28, 2019 •

edited

Loading