Introduce Fortran Test Framework and CTest Integration #29
Conversation
amstokely
force-pushed
the
feature/fortran_test_framework
branch
from
3292495 to
4177b09
Compare
obs2ioda-v2/README.md
Outdated
| @@ -22,11 +22,11 @@ If you have an environment preconfigured for `mpas-jedi`, simply source that env | |||
| ```bash | |||
| find . -name *libbufr* | |||
| ``` | |||
| 4. Next, run CMake to configure the build. Remember to specify the path to the NCEP BUFR library: | |||
| 4. Next, run CMake to configure the build. Specify the `CMAKE_BUILD_TYPE` option to set the build type. Currently, the supported types are `Release`, `RelWithDebInfo`, and `Debug`. Don't forget to include the path to the NCEP BUFR library: | |||
There was a problem hiding this comment.
Thank you for the clear install instructions, they are great! The build types 'Release' and 'Debug' seem intuitive, but it is less clear to me under what circumstances I should use 'RelWithDebInfo' instead of say 'Release'. Would it be worthwhile to add a comment on the use cases for the build types?
There was a problem hiding this comment.
@st-ncar You use RelWithDebInfo when you want to profile using profiling tools that require Debug symbols. I'll add a note about this to the Readme. Thanks for the suggestion!
There was a problem hiding this comment.
I added a table that Summarizes the properties and use cases for each supported CMake build type in the latest commit
amstokely
force-pushed
the
feature/fortran_test_framework
branch
2 times, most recently
from
4e143d2 to
4177b09
Compare
This PR introduces a Fortran testing framework and CTest integration to improve the testing and validation If the test is a memcheck test, the memcheck assert handler is used, else the standard assert handler is used.
amstokely
force-pushed
the
feature/fortran_test_framework
branch
from
fd57514 to
758407f
Compare
| integer, intent(in) :: expected, actual | ||
| integer, intent(out) :: status | ||
| procedure(assert_interface) :: assert_procedure | ||
| procedure(assert_interface), pointer :: assert_handler => assert |
There was a problem hiding this comment.
This assignment seems unnecessary because assert_handler gets reassigned on the next executable line.
Ditto for lines 125 and 149.
| public :: assertEqual | ||
| public :: assert_interface | ||
| public :: determine_test_type | ||
| public :: assert | ||
| public :: assert_memcheck |
There was a problem hiding this comment.
public is the default access. You need to use private if you want parts of the module to be private.
|
I don't understand the file hierarchy or the file nomenclature. Is Why is |
… Also combined test driver and test module to decrease file structure complexity.
…t subroutine exit(exit_code) is not part of the fortran standard and while it is implemented in ifort and gfortran, it is not implemented in nvfortran.
…ioda in a non-local directory.
| assert_handler => assert_procedure | ||
|
|
||
| call assert_handler(& | ||
| expected == actual, "expected='" // trim(expected) // "' actual='" // & |
There was a problem hiding this comment.
What is the expected behavior if the string variables expected and actual have leading spaces? At current, ' a' and 'a' would not be considered equal, but 'a ' and 'a' would. Maybe it would be more consistent if leading and trailing spaces were both ignored in the comparison? In this case, we should left-adjust and trim the expected and actual string before the comparison in assert_handler.
| if (.not. condition) then | ||
| status = 1 | ||
| write(*, '(A)') "Failed: " // message | ||
| stop 1 |
There was a problem hiding this comment.
This is a stylistic comment and can also be ignored. I find the distinction between assert and assert_log, and their signature enforced by the assert_interface a bit confusing. It seems like assert_log provides the basic functionality and assert could in principle call assert_log and terminate program execution if status == 1 (that way its status wouldn't be unused). I think we talked a bit about this during our chat and I forgot our conclusion, sorry if this is a duplicate. And as I said, this is stylistic, I think assert_log and assert work fine as is.
There was a problem hiding this comment.
I agree that assert should have a suffix given that assert_log does. I'll update this in my next commit
I agree that the file naming/structure is confusing. I changed the name of the module from fortran_test_framework to test_utils and moved the test program and test module into one file, which should significantly clean up the overall design of the new module. |
Summary:
This PR introduces a Fortran testing framework and CTest integration to improve the testing and validation process for the library. Key updates include an assertion module, unit tests for core features, and CMake functions for streamlined test management. To test these changes, run ctest in the build directory after compiling obs2ioda.