Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Using Catch2 instead of our custom unit test system #1575

Closed
drdanz opened this issue Mar 6, 2018 · 14 comments
Closed

Using Catch2 instead of our custom unit test system #1575

drdanz opened this issue Mar 6, 2018 · 14 comments

Comments

@drdanz
Copy link
Member

drdanz commented Mar 6, 2018

I made some tests porting YARP unit tests to Catch2, see https://github.com/drdanz/yarp_test_catch2

Porting seems to be really quick and easy, and the code is cleaner. The library is just one header file.
I think it is worth porting.

What do you think?

@drdanz drdanz added Component: Tests Type: Discussion Type: Refactor Involves refactoring some part of code labels Mar 6, 2018
@traversaro
Copy link
Member

👍

@traversaro
Copy link
Member

If it is doable without much effort, I would try to use catch 1 to avoid embedding a copy of the library (~500 kb) in YARP, as we could (at least in Ubuntu) use the version provided by apt :
https://repology.org/metapackage/catch/versions .

@lornat75
Copy link
Member

lornat75 commented Mar 6, 2018

I wonder if to take advantage of the features provided by catch we need to refactor the tests anyway, instead of porting "as is".

@drdanz
Copy link
Member Author

drdanz commented Mar 6, 2018

Porting "as is" is just enough to get everything working, I don't think a full refactor is needed.
Of course adding more tests is always welcome 😄

Also setting up a coverage infrastructure would be really useful

@lornat75
Copy link
Member

lornat75 commented Mar 6, 2018

I like "porting as is is just enough" 👍

@drdanz
Copy link
Member Author

drdanz commented Mar 6, 2018

About catch1, I made a quick test, everything seems compatible with catch from Debian, except for adding command line options (I used it in order to support the --yarp-verbose argument (verbose was already taken by catch), see drdanz/yarp_test_catch2@4b49f69

Also a FindCatch.cmake is required, since there is not one.

@lornat75 If you really want some refactor, one improvement could be not using "impl" in unit tests, or separate public libraries and impl unit tests. In this way we could actually filter out impl symbols from the ABI

@traversaro
Copy link
Member

Not blocking this activity in any way, but it may be worth mentioning that catch is not currently supporting parametrized tests, see catchorg/Catch2#850 .

@drdanz
Copy link
Member Author

drdanz commented Mar 6, 2018

That's one limit in the Catch library, but I'll add that yarp::os::impl::UnitTest does not support that as well...

@Nicogene
Copy link
Member

Nicogene commented Mar 6, 2018

Probably a silly question, Catch seems a nice testing framework, but why we do not migrate to our testing framework??

@drdanz
Copy link
Member Author

drdanz commented Mar 6, 2018

RTF is not an "unit testing framework", it is an "integration test framework" (I just made up the two definitions)

@Nicogene
Copy link
Member

Nicogene commented Mar 6, 2018

🤔 I see, probably RTF is an overkill for what we need.

@drdanz
Copy link
Member Author

drdanz commented Mar 6, 2018

@traversaro catch1 from system is now supported in master 🙈

I added a check on CATCH_VERSION_MAJOR that actually exists since Catch v2.1.2, therefore that part is left out even on Catch2 2.1.1 and previous releases even though they would support it. Anyway, not a real issue.

@traversaro
Copy link
Member

An additional reason for this: I recently added a new test for libYARP_dev, and the test was not added to CTest until I removed the build directory and started a fresh build.

@drdanz
Copy link
Member Author

drdanz commented Mar 22, 2019

THis was fixed long time ago. Closing

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants