-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Replace Catch with Doctest #1921
Conversation
Thanks for trying this out. Do you know of any other trade-offs (pros + cons) of doctest vs. Catch2? While SFML compile times are better with doctest, they're nowhere near the "20 times" or "many times" advertised in the FAQ -- which in turn makes me skeptical how accurate their other promises are. |
I have personally used doctest both at work and in some personal projects (currently private repositories). I've never needed any advanced feature, but it does the job well for all my testing needs. I have used Catch in the past but after switching to doctest I have no reason to go back, it is just faster.
Their claims are not bogus. Check the analyses that I posted in the OP -- Catch dominates a huge chunk of the first compilation process analysis, and doctest doesn't even appear in the second one! Note that my analyses and timings are for a full complete build of SFML, including examples and tests. The benchmarks on doctest are way more specialized. |
I remember trying out doctest the first time I heard of it at one of the author's presentations. I too noticed a perceivable build time improvement, especially when compared to Catch 1 (which was known for having bad build times back then). Although the library promotes writing the tests right next to the code I never really got used to that idea. I am not against replacing Catch2 with doctest if it offers many benefits to us given the current way our unit tests are written. |
I am not suggesting changing the way we write unit tests, I don't like putting them next to the code either. The benefits of this PR are strictly about improving compilation times and reducing CI load -- I think the benchmarks are quite impressive. |
Note that doctest doesn't need the workaround for MinGW introduced in #1898, as well. |
How much of a bottleneck is building tests? I'm slowly getting up to speed with how CI is being handled with SFML but it appears that by far the biggest bottleneck to PRs are the non-GitHub CI jobs that seemingly take 10x longer than the GitHub jobs. Does this project only get so many minutes of CI time per month that we're trying to optimize for? Does this PR meaningfully speed up those other jobs that currently take so long to complete? |
I have not measured the time it takes to perform a full CI run prior and after this PR. However, on my local machine a full rebuild of SFML is 4s faster after this PR. There are 47 CI checks. If we save a similar amount of time on a CI machine, we could be looking at around 188s of total time save. (@binary1248: any way we could measure that?) Regardless, I would ask a different question: if both Catch2 and doctest provide the functionality that we require, but one is clearly faster to compile than the other, and transitioning from one to the other is trivial... why would we ever choose to pick the slower one? |
d13d6bd
to
6726724
Compare
6726724
to
d418e8d
Compare
@eXpl0it3r, @Bromeon: could you approve this before we start making more contributions to the testing part of SFML (e.g. #1933 )? |
d418e8d
to
47a8af7
Compare
@ChrisThrasher are you okay if we merge this soon and have your PRs having to be adapted to doctest? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm OK with this change.
Are there more objections, in favor of Catch2? @ChrisThrasher @eXpl0it3r or so?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be removed again, unless MinGW struggles as well with Doctest
Lines 16 to 19 in cd1cc62
# Work around MinGW struggling to process a file as large as catch.hpp | |
if(MINGW) | |
target_compile_options(sfml-test-main PUBLIC -Wa,-mbig-obj) | |
endif() |
What we should keep in mind: the future of Doctest is not very clear at this point -- doctest/doctest#554 |
I'm fine adapting my MRs. Looks like a simple find-replace to make the switch.
I'm not excited about the prospect of adopting a library without a clear maintenance plan just to save a few seconds of compile time, but I won't object to this. |
47a8af7
to
04099bd
Compare
Done. I remember it didn't need big-obj, but let's see if CI agrees.
Ditto... reverting to Catch is always an option if we ever need to, it would be easy to do. |
Pretty cool to see these results! Indeed the benchmarks in doctest are synthetic - they measure the compile time of simple translation units including only the framework header and nothing from the stdlib. I'm quite hopeful that the framework has a future! |
This PR changes the testing framework from Catch to doctest, mostly to improve compilation times and reduce the load on CI machines. The results speak for themselves:
Compilation time analysis with Catch
Compilation time analysis with Doctest
This PR would make #1898 obsolete, but the work that has been done there is still greatly appreciated and the improvements brought by #1918 are still valid for doctest. Thanks!