Auto-discover unit tests #10922
Auto-discover unit tests #10922
Conversation
Myoldmopar
added
the
DoNotPublish
|
| DISCOVERY_TIMEOUT 30 | ||
| WORKING_DIRECTORY ${PROJECT_BINARY_DIR} | ||
| ) | ||
|
|
There was a problem hiding this comment.
Replaced that block of "manual" source code parsing with just a call to let gtest discover the tests and add them to ctest. Excellence.
| // surfShade.blind.slatAngInterpFac); | ||
| // surfShade.effGlassEmi = Interp(constrSh.effGlassEmi[surfShade.blind.slatAngIdxLo], | ||
| // constrSh.effGlassEmi[surfShade.blind.slatAngIdxHi], | ||
| // surfShade.blind.slatAngInterpFac); |
There was a problem hiding this comment.
The comment explains it, but basically, on my Apple Silicon, in a release build, this was throwing a segfault because the array indices were still -1 here. I just moved them to where they were assigned before the Material refactor, and it's all happy locally. We'll see how CI likes it.
| surfShade.blind.slatAngInterpFac); | ||
| surfShade.effGlassEmi = Interp(construction.effGlassEmi[surfShade.blind.slatAngIdxLo], | ||
| construction.effGlassEmi[surfShade.blind.slatAngIdxHi], | ||
| surfShade.blind.slatAngInterpFac); |
There was a problem hiding this comment.
Moved these back here, where they were before the Material refactor. Let's see.
There was a problem hiding this comment.
There was a test here that was disabled, and hadn't been run in a long time. (DISABLED_AirLoopNumTest) Since ctest is more aggressively reporting out disabled tests, I commented it out. Happy to get it back in if someone wants to spend time investigating the failure.
| main.cc) | ||
|
|
||
| set(test_dependencies energypluslib) | ||
| set(test_dependencies energypluslib energyplusapi) |
There was a problem hiding this comment.
I got rid of the separate energyplusapi_tests build. Just build the unit tests into a single test executable that depends on the lib and the api both being built. These are not optional things in the EnergyPlus build, so it should be fine for every platform, dev, machine, etc. (Famous last words)
| @@ -94,13 +94,16 @@ TEST_F(EnergyPlusFixture, TestTrendVariable) | |||
| EXPECT_DOUBLE_EQ(0.0, pluginManager.getTrendVariableValue(*state, trendVarIndex, 3)); | |||
| } | |||
|
|
|||
| TEST_F(EnergyPlusFixture, DISABLED_MultiplePluginVariableObjects) | |||
| TEST_F(EnergyPlusFixture, MultiplePluginVariableObjects) | |||
There was a problem hiding this comment.
This test is a bit invalid nowadays. I left what I felt was reasonable.
| @@ -1908,7 +1908,7 @@ TEST_F(EnergyPlusFixture, InitAirLoops_1AirLoop2Zones3ADU) | |||
| EXPECT_EQ(state->dataZoneEquip->ZoneEquipConfig(2).InletNodeAirLoopNum(1), 1); | |||
| } | |||
|
|
|||
| TEST_F(EnergyPlusFixture, DISABLED_AirLoop_ReturnFan_MinFlow) | |||
| TEST_F(EnergyPlusFixture, AirLoop_ReturnFan_MinFlow) | |||
There was a problem hiding this comment.
Enabled this and it's good to go!
| @@ -405,7 +405,7 @@ TEST_F(SizingAnalysisObjectsTest, PlantCoincidentAnalyObjTest) | |||
| EXPECT_TRUE(TestAnalysisObj.anotherIterationDesired); | |||
| } | |||
|
|
|||
| TEST_F(SizingAnalysisObjectsTest, DISABLED_LoggingSubStep4stepPerHour) | |||
| TEST_F(SizingAnalysisObjectsTest, LoggingSubStep4stepPerHour) | |||
There was a problem hiding this comment.
Enabled this and it's good to go!
| // life DISABLE_d anyway, so this is a net improvement. | ||
| if (std::getenv("CI")) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
Same thing as numThreads here. There's something super odd about the CI image that causes this to fail. I do not understand. I'm happy to let it run on everyone's personal machines regularly and not worry about this on CI.
| @@ -66,7 +66,7 @@ | |||
|
|
|||
| using namespace EnergyPlus; | |||
|
|
|||
| TEST_F(EnergyPlusFixture, DISABLED_WCEClear) | |||
| TEST_F(EnergyPlusFixture, WCEClear) | |||
There was a problem hiding this comment.
Enabled 3 tests here and they seem good to go!
|
All clean woo!!! OK, this is going in. Thanks to @kbenne for the discovery tip. If anyone notices any weirdness, obviously let me know. And if anyone wants to poke around at any of the tests I commented, that would be great. They haven't been running in various amounts of time, so they may need significant attention to get running again. |
TL;DR: I found a bunch of unit tests were being missed in ctest.exe runs, so I improved the test discovery.
While debugging and poking around, I realized that ctest wasn't picking up all the unit tests, Quick background:
energyplus_tests.exectesttest management.TESTandTEST_Fdeclarations that we then massage into ctest using theadd_testfunction.This has worked great for many years, but now some advanced things have trickled in, like parameterized unit tests. These use different Google Test macros, like
TEST_P. These work great when you runenergyplus_tests.exe, but they were not being picked up by our code that massages these into ctestadd_testcalls. So if you runctestlike I do, you miss out on running those unit tests.Thanks to @kbenne for pointing out that there is a more modern way of doing this that doesn't rely on parsing the code. Just call
gtest_discover_testsand point it to the built binary. It will programmatically grab all the unit tests declared so that we never miss any at all!This was really great, but it also changed the way the test output is emitted, and it forces reporting any disabled tests. Decent CI detects those and reports them to us, which I saw as an opportunity to go in and fix the failing tests. Or at least touch base with them. I ultimately fixed a few, and then commented a few back out since they weren't running anyway. At some point if someone wants to push on them further to get them running, that's great, but I'm not taking more time now.
As of now, develop sees 2926 tests, but this branch sees 2951. So a gain of 25 tests that were being missed by CI !!