Build out scaffolds for future unit tests#9506
Conversation
Myoldmopar
added
the
DoNotPublish
|
Windows CI failure is because it couldn't delete a temporary file, nothing to do with this. Linux release CI is the unrelated solar shading GPU diff. CI is fully happy here. |
Myoldmopar
left a comment
Myoldmopar
left a comment
There was a problem hiding this comment.
OK, this PR is hopefully ready to go in. It is not making functional changes, just scaffolding out and fleshing out unit tests for source files which were especially desperate for unit test coverage.
| DataSurfaces.hh | ||
| DataSystemVariables.cc | ||
| DataSystemVariables.hh | ||
| DataTimings.cc |
There was a problem hiding this comment.
Removed the internal timing sampling code -- left the elapsed time calculation as-is, for now. There is an opportunity to strip out some more Objexx and use c++ chrono built in to do that calculation instead, but I left that for later.
| build_type: Debug | ||
| cmake_extra_flags: -DLINK_WITH_PYTHON:BOOL=ON -DPython_REQUIRED_VERSION:STRING=3.8 -DPython_ROOT_DIR:PATH=~/.pyenv/versions/3.8.12/ -DBUILD_FORTRAN:BOOL=ON -DBUILD_TESTING:BOOL=ON -DENABLE_REGRESSION_TESTING:BOOL=OFF -DCOMMIT_SHA:STRING=$COMMIT_SHA -DENABLE_COVERAGE:BOOL=ON -DENABLE_GTEST_DEBUG_MODE:BOOL=OFF -DENABLE_PCH:BOOL=OFF | ||
| coverage_enabled: truec | ||
| coverage_enabled: true |
| const char *fluidNameSteam = "STEAM"; | ||
|
|
||
| PlantComponent *BoilerSpecs::factory(EnergyPlusData &state, std::string const &objectName) | ||
| BoilerSpecs *BoilerSpecs::factory(EnergyPlusData &state, std::string const &objectName) |
There was a problem hiding this comment.
The factory methods should return the concrete type. The plant structure will store a PlantComponent*, but this conversion should happen when the conversion is needed, not as this function exits. By returning the concrete type, a caller of the factory can use the concrete type without requiring a cast back to the concrete type. This has no functional effect on EnergyPlus itself.
| ThermalEN673Calc.hh | ||
| ThermalISO15099Calc.cc | ||
| ThermalISO15099Calc.hh | ||
| Timer.h |
There was a problem hiding this comment.
Removed the internal timing stuff per some discussion here: #9468. Note that this does not affect the user-facing runtime tracking that shows up in the output files. This is merely removing the compiler-define-dependent, manual, internal time tracking, that is not necessary given modern profiling tools.
|
|
||
| PlantComponent *EvapFluidCoolerSpecs::factory(EnergyPlusData &state, DataPlant::PlantEquipmentType objectType, std::string const &objectName) | ||
| EvapFluidCoolerSpecs * | ||
| EvapFluidCoolerSpecs::factory(EnergyPlusData &state, DataPlant::PlantEquipmentType objectType, std::string const &objectName) |
There was a problem hiding this comment.
Again just return the concrete type pointer so that we can call the factory methods and get a meaningful return value rather than just a nebulous PlantComponent*.
| return FuelTypeErrorsFound; | ||
| } | ||
|
|
||
| Real64 epElapsedTime() |
There was a problem hiding this comment.
Moved the epElapsedTime function to UtilityRoutines. I think this is worth a second pass to clean up the Objexx timing functions, which could lead to trimming down the Objexx library even further. But that would muddy this PR up, so that can just be 📌
| } | ||
|
|
||
| void progressHandler(EnergyPlusState state, int const progress) | ||
| void progressHandler(int const progress) |
There was a problem hiding this comment.
Cleaned up this runtime test script, fixing some things along the way and adding test coverage for the runtime.cc file.
| @@ -0,0 +1,101 @@ | |||
| // EnergyPlus, Copyright (c) 1996-2022, The Board of Trustees of the University of Illinois, | |||
There was a problem hiding this comment.
Several new and fleshed out unit test files here. Note that the purpose of this PR is to scaffold out the unit tests, which should make it easier for developers to add targeted checks while they are doing normal development for defects and features.
|
|
||
| namespace EnergyPlus { | ||
|
|
||
| TEST_F(EnergyPlusFixture, ExerciseBaseboardElectric) |
There was a problem hiding this comment.
Wherever possible I tried to avoid and/or minimize the IDF input strings in the unit test. Long term it would be amazing to just be able to operate on the objects themselves and not require input strings in the unit tests.
dareumnam
left a comment
dareumnam
left a comment
There was a problem hiding this comment.
This looks all great to me. I built it locally and ran tests successfully and CI is all happy here. This one looks fully clean and ready to go. I think this PR makes our life easier with unit testing. Thanks! @Myoldmopar
7 participants
Pull request overview
One of the burdens to improving unit test coverage is that there is a (sometimes quite large) initial effort to create the basic unit test scaffold for a source file or class. This PR simply builds out some scaffolds for files that have zero or minimal unit test coverage, to allow for higher test coverage to be added and the objects to be tested more thoroughly with minimal effort. Some other cleanups happened along the way, including removing the unused internal timing calculations. Note that the EnergyPlus elapsed time is still kept, just moved to UtilityRoutines. And the internal function call counting is still left in place for now.