Stale Tests

[Michael Betancourt]

There are a huge number of “unit” tests which really just test
regression behavior. These are particularly annoying because
there is absolutely no information about how the test content
was generated so that it can be updated when the code
necessitates it. This kind of test makes development a
pain in the ass, and at the very least any such stale file
needs to also include how it was generated.

Examples include

stan-dev/cmdstan/src/test/interface/matrix_output.csv

There are also the files in

stan-dev/stan/src/test/unit/io/test_csv_files

These are particularly frustrating as they depend on CmdStan
which introduces a circular dependency in the repositories. At
the same time, because they depend on static (usually static)
files they’ll pass even when there are inconsistencies.

Dan introduced all of these, so I think it’s fair that he provide
the code necessary to reproduce them.

[Bob Carpenter]

> On Mar 8, 2016, at 4:44 PM, Michael Betancourt <betan...@gmail.com> wrote:
>
> There are a huge number of “unit” tests which really just test
> regression behavior.

Agreed. I've never liked these tests. At the same time,
I see Daniel's desire to have something in place which will tip us
off if we break something by accident.

> These are particularly annoying because
> there is absolutely no information about how the test content
> was generated so that it can be updated when the code
> necessitates it.

I also agree there should be some indication of how to
update the tests if they were just auto-generated.

> This kind of test makes development a
> pain in the ass, and at the very least any such stale file
> needs to also include how it was generated.

By "stale", you mean the output has been changed on purpose?

Any output changes are potentially going to affect CmdStan
and the interfaces that depend on it.

> Examples include
>
> stan-dev/cmdstan/src/test/interface/matrix_output.csv
>
> There are also the files in
>
> stan-dev/stan/src/test/unit/io/test_csv_files
>
> These are particularly frustrating as they depend on CmdStan
> which introduces a circular dependency in the repositories. At
> the same time, because they depend on static (usually static)
> files they’ll pass even when there are inconsistencies.

...

Presumably they don't depend on CmdStan at test time for Stan.

I don't know what you mean by passing even when there are inconsistencies.
What's an inconsistency here?

- Bob

[Daniel Lee]

> On Mar 8, 2016, at 4:44 PM, Michael Betancourt <betan...@gmail.com> wrote:
>

> There are a huge number of “unit” tests which really just test
> regression behavior. These are particularly annoying because
> there is absolutely no information about how the test content
> was generated so that it can be updated when the code
> necessitates it. This kind of test makes development a
> pain in the ass, and at the very least any such stale file
> needs to also include how it was generated.

Agreed. They were originally put in place as regression tests, but you're right, these are hard to maintain. With the actual refactoring of the code, we can push a lot of these tests down into Stan and not have to do string comparisons any more.

(Although they were / are a pain in the ass, they've done their job in preventing bugs where our unit tests didn't have coverage. I'm not suggesting we keep them, but we should move forward.)

Something to note now, we should take what we pump into each of the writers more seriously now. They define part of the behavior.

>
> Examples include
>
> stan-dev/cmdstan/src/test/interface/matrix_output.csv
>
> There are also the files in
>
> stan-dev/stan/src/test/unit/io/test_csv_files
>
> These are particularly frustrating as they depend on CmdStan
> which introduces a circular dependency in the repositories. At
> the same time, because they depend on static (usually static)
> files they’ll pass even when there are inconsistencies.
>

I'll start with those. Is there an open issue? If not, I'll create one.

> Dan introduced all of these, so I think it’s fair that he provide
> the code necessary to reproduce them.
>
>

> --
> You received this message because you are subscribed to the Google Groups "stan development mailing list" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to stan-dev+u...@googlegroups.com.
> For more options, visit https://groups.google.com/d/optout.

[Michael Betancourt]

This kind of test makes development a
pain in the ass, and at the very least any such stale file
needs to also include how it was generated.

By "stale", you mean the output has been changed on purpose?

Any output changes are potentially going to affect CmdStan
and the interfaces that depend on it.

Examples include

stan-dev/cmdstan/src/test/interface/matrix_output.csv

There are also the files in

stan-dev/stan/src/test/unit/io/test_csv_files

These are particularly frustrating as they depend on CmdStan
which introduces a circular dependency in the repositories.  At
the same time, because they depend on static (usually static)
files they’ll pass even when there are inconsistencies.

...

Presumably they don't depend on CmdStan at test time for Stan.

I don't know what you mean by passing even when there are inconsistencies.
What's an inconsistency here?

The problem is mainly with the stan_csv_reader, which is in Stan.

The tests depend on CmdStan output isolated to constant text files

so they always pass.  This itself is fine as the tests are just testing

behavior with respect to those files.  

But then if there are any changes to, say, the samplers, then there 

is an inconsistency between what CmdStan will output and what is 

in those text files.  This will lead to CmdStan tests to then fail unless

Stan gets updated.  The tests are helping nothing in this case.