Tests: Convert test headers to compilation files#112041
Conversation
|
I'd be interested to see a compile time comparison before and after this PR. A middle ground could be acceptable, where e.g. we split the one test file into ~10 compile units. This may still leave us with slightly worse overall thread time, but should solve the 'critical path' problem. |
|
@Ivorforce Better yet, I've just made sure this accounts for SCU builds as well. It does make sense to replicate the old behavior when already expecting a single compilation unit. It's a bit hacky, as SCU registration/identification is pretty hardcoded atm, but it gets the job done For standard builds, it'd only be a loss for initial compilations, otherwise the only tests that'd be recompiled are those that had differences in their headers. I was going to trim down the headers in stuff like |
a80bd58 to
404f807
Compare
293fcfb to
2fb0d82
Compare
akien-mga
left a comment
There was a problem hiding this comment.
Looks great to me, and I can confirm it reduces incremental rebuild time by a few seconds when test_main.cpp would be recompiled.
• Excludes module tests, as they'd be a more involved process
2fb0d82 to
5482b9e
Compare
|
Went ahead and un-suppressed the |
AThousandShips
left a comment
There was a problem hiding this comment.
Looks great! Just two questions
It's no secret that
test_main.cppis, by a significant margin, the most header-intensive file in the repository. This is on account of it including the contents of every single test, as they've been setup in headers. Moving to compilation units would be preferable, but that means that the tests themselves would fail to link, as a result of those scripts being isolated. In trying to look for workarounds, I took to the doctest repo itself and found out how they managed this process in CMake.One SCons-reimplementation later, and everything actually works! Now all a designated test file has to do is include the define
TEST_FORCE_LINK()and pass the name of the file; generator scripts will take care of the rest. No need to redeclare our entire library setup or an immediate need to resolve all encapsulation issues, so the transition isn't nearly as messy as I thought it would be.There's a lot of changes made in this PR, but that's an inevitability when retooling this many files to be compilation files instead of headers. Several headers had to be added to compilation files, as they no longer included everything transitively. Transitive includes are still somewhat of an issue thanks to
test_macros.h, but that can be handled in a followup PR.Two tests had to be suppressed after the conversion:EDIT: Tested out on a separate branch and compilation worked again, so this might've been a false positive. Will keep an eye on.[Variant] Utility functionsand[ClassDB] Add exposed classes, builtin types, and global enums. The silver lining is I'm now reasonably certain that these two are the culprits behind our tests occasionally hanging for hours, as both consistently recreate the failure states when active. Though for whatever reason, the results are often amplified, with both cases outright segfaulting. Both absolutely warrant a more thorough dive, but that is beyond the scope of this PR & most likely would go over my head.NOTE: This conversion excludes module tests, as they'd be a more involved process than the base tests. As such,
#include "modules/modules_tests.gen.h"remains unchanged, meaningtest_main.cppstill has some header bloat for the time being. Now that I know this is possible, a followup PR will take care of the module tests once this is merged.