[soci-users] RFD: Using CATCH for tests

[Vadim Zeitlin]

Hello,

While we're discussing other "strategic" things, I'd like to also return
to something that was mentioned a couple of times in the past but without
any definitive conclusion, unless I missed it: could we start using CATCH
(http://catch-lib.net/) in the unit tests?

Currently the tests use assert() which is far from ideal (I could give
details but I'll skip this for now because I think everybody realizes the
problems with assert() already). The only advantage of the current approach
is that it has no third party dependencies. I'd like to use a header-only
testing framework to preserve this advantage as much as possible while also
getting all the nice features you'd expect from such framework and CATCH is
the best candidate I know of.


Assuming there are no objections to using it, I'd like to add catch.hpp
(https://github.com/philsquared/Catch/blob/master/single_include/catch.hpp)
directly to SOCI repository, e.g. as tests/catch.hpp. I don't think it's
worth the trouble to use a submodule for it, but we could do this too if
people prefer it.

I am less sure about how to handle the transition from assert() to CATCH.
One possibility would be to just globally replace all asserts with
REQUIRE() macros. This would be suboptimal (if only because some of the
asserts should probably be replaced with CHECK(), to allow the tests to
continue if one of them fails), but should be relatively simple to do. And
if we do this, we should probably also move all the files from tests/assert
one directory level up, as the "assert" part wouldn't make any sense any
more.

Another possibility would be to start a parallel tests hierarchy in
tests/catch. I'm not too kin on it, mostly because I don't like duplication
but also because this would require changing the CMake files and I'd rather
avoid this.

Finally, it would be possible to just start using CATCH for the checks
without changing anything in the existing ones. I don't like this one too
much neither because it's going to be confusing to have different macros
used in different places, but it's definitely the simplest one.

What would be the preferred approach? Or maybe someone sees a better one,
not listed above? And, of course, do we have a consensus for using CATCH in
the first place?

Thanks in advance for your thoughts,
VZ

[Mateusz Loskot]

Post by Vadim Zeitlin
While we're discussing other "strategic" things, I'd like to also return
to something that was mentioned a couple of times in the past but without
any definitive conclusion, unless I missed it: could we start using CATCH
(http://catch-lib.net/) in the unit tests?

I haven;t requested any discussion, so no conclusion indeed.
I just put the CATCH on the https://github.com/SOCI/soci/wiki/Roadmap and
was going to start converti,ng/writing the tests, then show it to the
public for review.

Post by Vadim Zeitlin
Currently the tests use assert() which is far from ideal (I could give
details but I'll skip this for now because I think everybody realizes the
problems with assert() already). The only advantage of the current approach
is that it has no third party dependencies. I'd like to use a header-only
testing framework to preserve this advantage as much as possible while also
getting all the nice features you'd expect from such framework and CATCH is
the best candidate I know of.

Yes.

Post by Vadim Zeitlin
Assuming there are no objections to using it, I'd like to add catch.hpp
(https://github.com/philsquared/Catch/blob/master/single_include/catch.hpp)
directly to SOCI repository, e.g. as tests/catch.hpp. I don't think it's
worth the trouble to use a submodule for it, but we could do this too if
people prefer it.

Sure. I'd create a branch in the upstream repo, so we can all test and
contribute to the new tests transition.
Once we're happy, we merge to develop (soon to be master :))

Post by Vadim Zeitlin
I am less sure about how to handle the transition from assert() to CATCH.
One possibility would be to just globally replace all asserts with
REQUIRE() macros. This would be suboptimal (if only because some of the
asserts should probably be replaced with CHECK(), to allow the tests to
continue if one of them fails), but should be relatively simple to do.

I believe in single assert per test routine/case...see below.

Post by Vadim Zeitlin
And
if we do this, we should probably also move all the files from tests/assert
one directory level up, as the "assert" part wouldn't make any sense any
more.
Another possibility would be to start a parallel tests hierarchy in
tests/catch. I'm not too kin on it, mostly because I don't like duplication
but also because this would require changing the CMake files and I'd rather
avoid this.

My plan was to keep the current tests and start (re)writing with CATCH
in new folder: tests/integrate.
(My other plan was to create tests/unit where I aimed to cover common
API with plain unit tests.)

Finally, along the transition, I had an idea to refactor and clean up
the tests slightly to test single thing
per routines/cases, as short as possible, with Arrange/Act/Assert
principle in mind.
Perhaps some modularisation and groupping would be a good idea too.

Post by Vadim Zeitlin
Finally, it would be possible to just start using CATCH for the checks
without changing anything in the existing ones. I don't like this one too
much neither because it's going to be confusing to have different macros
used in different places, but it's definitely the simplest one.

I don't want to get rid of the assert-based tests before CATCH-based
tests cover what is already covered.
Removal of the assert tests should be the last step before merging the
PR/branch to master.

Post by Vadim Zeitlin
What would be the preferred approach? Or maybe someone sees a better one,
not listed above?

Having explained my points above, I don't think that find & replace
assert with CATCH macro is the way to go.

Post by Vadim Zeitlin
And, of course, do we have a consensus for using CATCH in the first place?

+1 from me, but even if we don't have concensus now, we can still
build new CATCH-based tests,
while keeping the assert ones, once it's finished and working, the
CATCH will catch, I believe :)

Best regards,

[Vadim Zeitlin]

On Thu, 26 Mar 2015 20:17:27 +0100 Mateusz Loskot <***@loskot.net> wrote:

ML> > Assuming there are no objections to using it, I'd like to add catch.hpp
ML> > (https://github.com/philsquared/Catch/blob/master/single_include/catch.hpp)
ML> > directly to SOCI repository, e.g. as tests/catch.hpp. I don't think it's
ML> > worth the trouble to use a submodule for it, but we could do this too if
ML> > people prefer it.
ML>
ML> Sure. I'd create a branch in the upstream repo, so we can all test and
ML> contribute to the new tests transition.

As you've noticed, currently there is catch-tests in my fork, but I could
push it to SOCI/soci, of course. The trouble is that it doesn't do what you
prefer...

ML> > I am less sure about how to handle the transition from assert() to CATCH.
ML> > One possibility would be to just globally replace all asserts with
ML> > REQUIRE() macros. This would be suboptimal (if only because some of the
ML> > asserts should probably be replaced with CHECK(), to allow the tests to
ML> > continue if one of them fails), but should be relatively simple to do.
ML>
ML> I believe in single assert per test routine/case...see below.

Sorry, this is unworkable. Currently, i.e. with catch-tests and Firebird
we have

All tests passed (1851 assertions in 54 test cases)

i.e. an average of more than 34 assertions per test. Multiplying the number
of test cases by 34 is not going to help.

Of course, the current tests are not well-organized and there is ample
scope for improvement here. And we definitely should be splitting some test
cases in sections using the eponymous macro. But we still won't have a
single assert per test case or even per section. But why should this be a
goal in the first place? I really don't see any good reasons to require
it.


ML> My plan was to keep the current tests and start (re)writing with CATCH

This would be my preference as well, in theory, but looking at the amount
of the existing testing code, I realized it would take an inordinate time
to do it, so it's just not going to happen in practice. Hence I decided to
do what I can to improve things right now instead of hoping that the best
case somehow happens on its own in the future...

ML> in new folder: tests/integrate.

What would be the meaning of integration tests for SOCI? I.e. integration
of what with what would they be testing? I am not sure how can we write
more than just unit tests for a library like it.

ML> Finally, along the transition, I had an idea to refactor and clean up
ML> the tests slightly to test single thing
ML> per routines/cases, as short as possible, with Arrange/Act/Assert
ML> principle in mind.
ML> Perhaps some modularisation and groupping would be a good idea too.

Yes, it would be definitely nice to streamline all this. But,
realistically, who is going to do it and when is it going to happen?


ML> Having explained my points above, I don't think that find & replace
ML> assert with CATCH macro is the way to go.

OK, as you know I've already done this in meanwhile. I actually didn't
want to start on this, I just wanted to add a new test (for checking the
error messages) using CATCH. But it turned out that I couldn't do it
without switching all the rest to CATCH first because I had to use
TEST_CASE or TEST_CASE_METHOD for all the test functions to run them, so I
started doing this and then was annoyed by seeing CATCH messages about N
tests passing but 0 checks being verified, so I also went ahead and did
replace assert()s with CHECKs.

So the question now is whether this makes things better or worse. IMHO it
does improve them, as the commit message says, using CATCH:

- Provides information about the failed test and the values of variables in it.
- Allows to continue running the tests even if one of them fails.
- Allows to run just some of the tests with flexible selection mechanism.

and all this is pretty convenient already. Additionally, it allows to write
new tests using CATCH and organizing them better, which was the original
impetus for this change.

It took me a couple of hours to make the changes so far and I'll probably
need at least as much to update the other backends (so far I've only done
it for Firebird), so I'd rather not do it if we don't want to replace
asserts with CATCH CHECK()s. But IMHO it would be a waste, the benefits
above are real and can be had today without any drawbacks (AFAICS).

The possibilities I see are:

1. Throw away catch-tests branch and start adding new tests using CATCH
in tests/unit.
+ This would allow cleaner tests, eventually.
- This still needs more work, notably at CMake level (which I try to
avoid as much as possible personally).
- We will be stuck with inconvenient to use/debug assert tests for
the observable future.
- It will also be confusing to have two parallel tests, we'd have to
explain people submitting PRs that they need to update this one and
not the other one, probably more than once.

2. Complete the work done for Firebird on catch-tests branch for the other
backends and merge it.
+ This is the fastest way to get a working CATCH-based test suite with
all the advantages listed above.
- I don't see any, really.

3. Improve the tests on the catch-tests branch to make them more modular,
better organized etc.
+ This would be the best outcome.
- Requires more work than I or, I suspect, anybody else, can spend on it
right now.

I would prefer to do (2) as debugging assert failures in the current tests
is painful and I really the more detailed error messages CATCH gives us in
case of failure, so for me the benefit of getting them a.s.a.p. seems
pretty important. But if there is a strong opposition to doing it, I'd go
with (1).

Please let me know what do you think,
VZ

[Mateusz Loskot]

Post by Vadim Zeitlin
ML> > I am less sure about how to handle the transition from assert() to CATCH.
ML> > One possibility would be to just globally replace all asserts with
ML> > REQUIRE() macros. This would be suboptimal (if only because some of the
ML> > asserts should probably be replaced with CHECK(), to allow the tests to
ML> > continue if one of them fails), but should be relatively simple to do.
ML>
ML> I believe in single assert per test routine/case...see below.
Sorry, this is unworkable. Currently, i.e. with catch-tests and Firebird
we have
All tests passed (1851 assertions in 54 test cases)
i.e. an average of more than 34 assertions per test. Multiplying the number
of test cases by 34 is not going to help.
Of course, the current tests are not well-organized and there is ample
scope for improvement here. And we definitely should be splitting some test
cases in sections using the eponymous macro. But we still won't have a
single assert per test case or even per section. But why should this be a
goal in the first place? I really don't see any good reasons to require it.

Not litreally, I meant single assert as testing one thing per test routine.
Currently, tests are bundled together around test data/table.

Post by Vadim Zeitlin
ML> My plan was to keep the current tests and start (re)writing with CATCH
This would be my preference as well, in theory, but looking at the amount
of the existing testing code, I realized it would take an inordinate time
to do it, so it's just not going to happen in practice. Hence I decided to
do what I can to improve things right now instead of hoping that the best
case somehow happens on its own in the future...

Yes, that's why it hasn't happened yet, my plan required more resource.
Your plan has slightly different goal and approach, and I'm support it too.

Post by Vadim Zeitlin
ML> in new folder: tests/integrate.
What would be the meaning of integration tests for SOCI? I.e. integration
of what with what would they be testing? I am not sure how can we write
more than just unit tests for a library like it.

We don't have unit tests.
The tests we have are integration tests.
I just had an idea of writing unit tests for the API, with mockery.
Hence, the distinction in the folders.

Post by Vadim Zeitlin
ML> Finally, along the transition, I had an idea to refactor and clean up
ML> the tests slightly to test single thing
ML> per routines/cases, as short as possible, with Arrange/Act/Assert
ML> principle in mind.
ML> Perhaps some modularisation and groupping would be a good idea too.
Yes, it would be definitely nice to streamline all this. But,
realistically, who is going to do it and when is it going to happen?

No, not in forseen future, I'm afraid.
I only explained my CATCH plans/ideas in the roadmap in some more details.

Post by Vadim Zeitlin
ML> Having explained my points above, I don't think that find & replace
ML> assert with CATCH macro is the way to go.

I should have added: "way to go to get where I had planned to" :)
Not that I'm disliking your proposal, no.
I just had some different goals.

Post by Vadim Zeitlin
OK, as you know I've already done this in meanwhile. I actually didn't
want to start on this, I just wanted to add a new test (for checking the
error messages) using CATCH. But it turned out that I couldn't do it
without switching all the rest to CATCH first because I had to use
TEST_CASE or TEST_CASE_METHOD for all the test functions to run them, so I
started doing this and then was annoyed by seeing CATCH messages about N
tests passing but 0 checks being verified, so I also went ahead and did
replace assert()s with CHECKs.
So the question now is whether this makes things better or worse. IMHO it
- Provides information about the failed test and the values of variables in it.
- Allows to continue running the tests even if one of them fails.
- Allows to run just some of the tests with flexible selection mechanism.
and all this is pretty convenient already. Additionally, it allows to write
new tests using CATCH and organizing them better, which was the original
impetus for this change.
It took me a couple of hours to make the changes so far and I'll probably
need at least as much to update the other backends (so far I've only done
it for Firebird), so I'd rather not do it if we don't want to replace
asserts with CATCH CHECK()s. But IMHO it would be a waste, the benefits
above are real and can be had today without any drawbacks (AFAICS).

I agree and I'm supportive about merging your efforts.

Post by Vadim Zeitlin
1. Throw away catch-tests branch and start adding new tests using CATCH
in tests/unit.

Nah, no resource.

Post by Vadim Zeitlin
2. Complete the work done for Firebird on catch-tests branch for the other
backends and merge it.

Let's stick to this one, IMHO.

Thanks for the efforts Vadim!

Best regards,

[Vadim Zeitlin]

On Thu, 26 Mar 2015 23:46:18 +0100 Mateusz Loskot <***@loskot.net> wrote:

ML> Not litreally, I meant single assert as testing one thing per test routine.
ML> Currently, tests are bundled together around test data/table.

Ah, yes, I certainly agree with this, sorry for the misunderstanding.

ML> > What would be the meaning of integration tests for SOCI? I.e. integration
ML> > of what with what would they be testing? I am not sure how can we write
ML> > more than just unit tests for a library like it.
ML>
ML> We don't have unit tests.
ML> The tests we have are integration tests.
ML> I just had an idea of writing unit tests for the API, with mockery.
ML> Hence, the distinction in the folders.

Interesting, it looks like we're not using the same terminology. For me
the simple tests directly exercising the library API are the unit tests.
But well, like all terminological discussions, this one is probably not
very interesting.

ML> I agree and I'm supportive about merging your efforts.
...
ML> > 2. Complete the work done for Firebird on catch-tests branch for the other
ML> > backends and merge it.
ML>
ML> Let's stick to this one, IMHO.

OK, thanks for your support! I won't be able to finish this today, but
I'll try to do it tomorrow or, if this fails too, early next week. One way
or the other, I really want to get CATCH in.

I will also try to find the time for splitting the tests in multiple
files, having a single ~4KLoC common-tests.h header is rather annoying. But
I don't promise this...

Regards,
VZ

[Mateusz Loskot]

Post by Vadim Zeitlin
ML> I agree and I'm supportive about merging your efforts.
...
ML> > 2. Complete the work done for Firebird on catch-tests branch for the other
ML> > backends and merge it.
ML>
ML> Let's stick to this one, IMHO.
OK, thanks for your support! I won't be able to finish this today, but
I'll try to do it tomorrow or, if this fails too, early next week. One way
or the other, I really want to get CATCH in.

No problem, sounds good.
Last night I resurrected the 3.2.3 efforts, there is a few tricky
cherry picks to make
and I'm good to release, not later than by the end of this month.

Post by Vadim Zeitlin
I will also try to find the time for splitting the tests in multiple
files, having a single ~4KLoC common-tests.h header is rather annoying. But
I don't promise this...

Sure, we do our best.

Best regards,

[Vadim Zeitlin]

On Fri, 27 Mar 2015 10:05:32 +0100 Mateusz Loskot <***@loskot.net> wrote:

ML> > OK, thanks for your support! I won't be able to finish this today, but
ML> > I'll try to do it tomorrow or, if this fails too, early next week. One way
ML> > or the other, I really want to get CATCH in.
ML>
ML> No problem, sounds good.

OK, this is done now, see https://github.com/SOCI/soci/pull/298

I'd like to merge this soon and then also rename/move the tests as the
"assert" part, which annoyed me even before, just totally won't make any
sense after switching to CATCH.

My favourite solution would be to just move the tests one level up in the
file system, i.e. get rid of the subdirectory completely. But we could also
keep the hierarchy and just rename it to something else, especially if
somebody has any good proposals for the subdirectory name ("catch" doesn't
count, because it doesn't really matter what do the tests use for their
checks, the name should rather describe what do they do and not how).

Thanks,
VZ

[Mateusz Loskot]

Post by Vadim Zeitlin
ML> > OK, thanks for your support! I won't be able to finish this today, but
ML> > I'll try to do it tomorrow or, if this fails too, early next week. One way
ML> > or the other, I really want to get CATCH in.
ML>
ML> No problem, sounds good.
OK, this is done now, see https://github.com/SOCI/soci/pull/298
I'd like to merge this soon

+1

Post by Vadim Zeitlin
and then also rename/move the tests as the
"assert" part, which annoyed me even before, just totally won't make any
sense after switching to CATCH.

+1

Post by Vadim Zeitlin
My favourite solution would be to just move the tests one level up in the
file system, i.e. get rid of the subdirectory completely. But we could also
keep the hierarchy and just rename it to something else, especially if
somebody has any good proposals for the subdirectory name ("catch" doesn't
count, because it doesn't really matter what do the tests use for their
checks, the name should rather describe what do they do and not how).

Two options which are equally good to me:
1. Move tests/assert/* one level up to tests/
2. Rename tests/assert to tests/{system|functional|integrate}, which may open
the structure for other test categories (e.g. plain unit tests that
don't test backends functional
integration with any physical DB env).

Please, choose whichever suits you.

Best regards,

[Vadim Zeitlin]

On Fri, 27 Mar 2015 18:25:04 +0100 Mateusz Loskot <***@loskot.net> wrote:

ML> On 27 March 2015 at 17:53, Vadim Zeitlin <vz-***@zeitlins.org> wrote:
ML>
ML> > My favourite solution would be to just move the tests one level up in the
ML> > file system, i.e. get rid of the subdirectory completely. But we could also
ML> > keep the hierarchy and just rename it to something else, especially if
ML> > somebody has any good proposals for the subdirectory name ("catch" doesn't
ML> > count, because it doesn't really matter what do the tests use for their
ML> > checks, the name should rather describe what do they do and not how).
ML>
ML> Two options which are equally good to me:
ML> 1. Move tests/assert/* one level up to tests/
ML> 2. Rename tests/assert to tests/{system|functional|integrate}, which may open
ML> the structure for other test categories (e.g. plain unit tests that
ML> don't test backends functional
ML> integration with any physical DB env).
ML>
ML> Please, choose whichever suits you.

In the absence of any objections/other proposals, I chose (1) and
merged my catch-tests branch in develop now. I didn't have time to further
reorganize the tests yet because I wanted to submit
https://github.com/SOCI/soci/pull/302 as soon as possible, as this is what
we really need, but I hope to do it in the future.

I think that ideal would be to stop building all the different
soci_xxx_test programs and build just a single soci_test which would
integrate all the backends and run the common tests for all of them by
default -- but allow to choose just a single (or several) backend(s) to use
from the command line. But this would require a lot of changes...

Regards,
VZ

[Mateusz Loskot]

Post by Vadim Zeitlin
ML>
ML> > My favourite solution would be to just move the tests one level up in the
ML> > file system, i.e. get rid of the subdirectory completely. But we could also
ML> > keep the hierarchy and just rename it to something else, especially if
ML> > somebody has any good proposals for the subdirectory name ("catch" doesn't
ML> > count, because it doesn't really matter what do the tests use for their
ML> > checks, the name should rather describe what do they do and not how).
ML>
ML> 1. Move tests/assert/* one level up to tests/
ML> 2. Rename tests/assert to tests/{system|functional|integrate}, which may open
ML> the structure for other test categories (e.g. plain unit tests that
ML> don't test backends functional
ML> integration with any physical DB env).
ML>
ML> Please, choose whichever suits you.
In the absence of any objections/other proposals, I chose (1) and
merged my catch-tests branch in develop now. I didn't have time to further
reorganize the tests yet because I wanted to submit
https://github.com/SOCI/soci/pull/302 as soon as possible, as this is what
we really need, but I hope to do it in the future.

Vadim,

It looks very good.
Thank you for your efforts!

Post by Vadim Zeitlin
I think that ideal would be to stop building all the different
soci_xxx_test programs and build just a single soci_test which would
integrate all the backends and run the common tests for all of them by
default -- but allow to choose just a single (or several) backend(s) to use
from the command line. But this would require a lot of changes...

This is an interesting idea and I support it.
It would simplify maintenance and CMake configuration
Do you mind to open an Issue for that?
I'm not sure how to combine common and specific tests
for all backends with CATCH, but I can do CMake clean up.

Best regards,