Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Refactor tests to remove dependencies on internal models #594

Merged
merged 4 commits into from
Aug 7, 2023
Merged

Refactor tests to remove dependencies on internal models #594

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
a676c5d
Useful notes on example case differences (and whitespace unification)
mattw-nws Jul 28, 2023
70b7449
Refactored tests to use BMI test models
mattw-nws Jul 31, 2023
32958d4
Actually, this is better, especially since we will want 0 extra steps…
mattw-nws Aug 1, 2023
b348075
Removed test_bmi_c from test_unit tests since it doesn't seem to actu…
mattw-nws Jul 31, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -143,6 +143,12 @@ add_executable(ngen
src/NGen.cpp
)

# Natively support BMI C++ modules and pre-compile in the test_bmi_cpp mock/example.
set(TEST_BMI_CPP_DIR ${PROJECT_SOURCE_DIR}/extern/test_bmi_cpp)
git_update_submodule(${PROJECT_SOURCE_DIR}/extern/bmi-cxx)
add_subdirectory(${TEST_BMI_CPP_DIR} ${TEST_BMI_CPP_DIR}/cmake_build)
add_dependencies(ngen testbmicppmodel) # because our demo realizations will use it

# Handle several steps for BMI C library logic and dependencies, at top level, if functionality is turned on
if(BMI_C_LIB_ACTIVE)
# Define associated preprocessor directive
Expand Down
1 change: 1 addition & 0 deletions extern/test_bmi_cpp/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


include(GNUInstallDirs)

Expand Down
3 changes: 3 additions & 0 deletions test/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,6 +104,7 @@ add_test(test_realization_config
NGen::core_mediator
NGen::forcing
libudunits2
testbmicppmodel
${NETCDF_LIBRARIES}
)

Expand Down Expand Up @@ -358,6 +359,7 @@ add_test(
NGen::geojson
NGen::realizations_catchment
libudunits2
testbmicppmodel
${NETCDF_LIBRARIES}
)

Expand Down Expand Up @@ -394,5 +396,6 @@ add_test(
NGen::geojson
NGen::realizations_catchment
libudunits2
testbmicppmodel
${NETCDF_LIBRARIES}
)
Loading
Loading