Refactor tests to remove dependencies on internal models #594
Conversation
… to get test_bmi_cpp for example realization config purposes.
…ally add any value to have more than one module tested here, and adding it would require different build flags to ngen. Using only the test_bmi_cpp mock, it would be relatively easy in the future to make Bmi_Formulation less "abstract" and support C++ BMI models not in a shared library, removing the need to compile the module in extern explicitly. Conflicts: .github/workflows/test_and_validate.yml
| "], " | ||
| "\"forcing\": { " | ||
| "\"file_pattern\": \".*{{ID}}.*.csv\", " | ||
| "\"file_pattern\": \".*{{ID}}.*.csv\", " // DIFF from Ex.1 |
There was a problem hiding this comment.
Are these comments placed to get git diff to behave sensibly? That's the only possibility I can infer from them
There was a problem hiding this comment.
Ah... those are I suppose vestiges from a676c5d, where I was trying to capture how the examples differed from one another, to determine what the tests were actually testing and hence what needed to be preserved. Still somewhat useful as road markers, since the four giant strings aren't easily comparable. (I recommend cutting example 1 and pasting it after example 4 if you want, that's how I compared them to get those notes.)
| @@ -27,6 +27,7 @@ target_include_directories(testbmicppmodel PRIVATE ../bmi-cxx/ ) | |||
| set_target_properties(testbmicppmodel PROPERTIES VERSION ${PROJECT_VERSION}) | |||
|
|
|||
| set_target_properties(testbmicppmodel PROPERTIES PUBLIC_HEADER include/test_bmi_cpp.hpp) | |||
| set_target_properties(testbmicppmodel PROPERTIES CXX_VISIBILITY_PRESET default) | |||
There was a problem hiding this comment.
From my perspective, this is a stop-gap toward actually export-marking the relevant exported symbols explicitly, to demonstrate the practice that we should be encouraging from real models that we integrate.
There was a problem hiding this comment.
Yes... to elaborate, when I added add_subdirectory(test_bmi_cpp...) to CMakeLists.txt, it was compiling with -fvisibility=hidden for reasons that weren't fully clear... it also wasn't fully clear why it wasn't doing that when building test_bmi_cpp with cmake standalone. Arguably, including that flag and having the desired public symbols explicitly marked (as @PhilMiller suggests) are the best practice, though there's little or nothing in this test module that isn't intended to be visible.
|
Not for this PR, but those embedded configurations would be an ideal use of C++ raw string literals |
Shifts unit tests to use BMI test mock models instead of legacy internal models. This lays the groundwork for #562 and additional legacy code removal
Additions
test_bmi_cppis now used for several unit tests in Formulation_Manager_Test.cpptest_bmi_cppwithout any additional flags#defines from Bmi_Formulation.hpp and AorcForcing.hpp (the latter was the case already but it wasn't explicitly included)Removals
tshirt,tshirt_c,simple_lumpedor anything that requireset_calcChanges
test_bmi_cppis now built as a dependency. The build should be the same as the usual way (e.g. the artifacts go to the same place) and shouldn't interfere with manual builds either.Testing
Screenshots
Notes
Todos
Bmi_Formulationinstead ofBmi_Module_Formulation, which would remove the required weirdness of building the plugin shared library version of test_bmi_cpp.Checklist
Testing checklist (automated report can be put here)
Target Environment support