diff --git a/doc/design/pfunit_testing.rst b/doc/design/pfunit_testing.rst new file mode 100644 index 0000000000..57e7e496dd --- /dev/null +++ b/doc/design/pfunit_testing.rst @@ -0,0 +1,37 @@ +.. sectnum:: + +.. contents:: + +================================== + Overview of this design document +================================== + +This document describes various design considerations for writing pFUnit-based unit tests. + +================= + Handling errors +================= + +When building the unit tests, we replace the standard ``shr_abort_mod`` with a version that throws a pFUnit exception instead of actually aborting. This is important for expected-error testing, where we want to check that the code aborts when it should: by throwing a pFUnit exception, we can check for this exception in the test. If we actually aborted, we wouldn't be able to catch and check this, and all remaining tests in the given module would be skipped. + +This behavior can sometimes be a problem, though, because Fortran doesn't have real exception handling. So when a pFUnit exception is "thrown", the code continues along as if nothing abnormal happened. This can lead to later errors, such as accessing unallocated variables, array bounds exceptions, floating point exceptions, etc. This means that you may not be able to do some expected error testing that you'd like to do, or in order to do this expected error testing, you'll need to add some code to prevent errors after calling ``endrun``. (This code after ``endrun`` won't be executed in the production case, but would be executed in unit testing.) + +In unit test-specific code, if we detect an unexpected error - which in this case would typically indicate an issue in the unit test code, rather than an issue in the production code - we use different techniques in different places. Where possible, we prefer calling ``shr_sys_abort`` if an unexpected error occurs in unit test-specific code. However, there are some situations where this is problematic, and we instead use ``stop 1``. (Note that ``stop 1`` causes the program to abort with a non-zero error code, which pFUnit interprets as a test failure, as is desired in these cases; in contrast, ``stop`` with no argument, an argument of 0, or a string argument causes the program to abort with a zero error code, which pFUnit interprets as a pass, which is often not what you want.) Some situations where we use ``stop 1`` instead of ``shr_sys_abort`` are: + +1. Unit test-specific code where throwing a pFUnit exception and continuing will lead to cryptic errors later (e.g., array bounds exceptions, etc.). + +2. Code called using pFUnit's ``EXTRA_FINALIZE`` technique - i.e., code called at the end of all tests in a given pFUnit executable. Throwing a pFUnit exception in these cases doesn't lead to a FAIL result, so we use a ``stop 1`` to ensure that we get a FAIL result if there's a problem in this finalization. + +(See https://github.com/ESCOMP/CTSM/pull/3017/commits/c4377dda9b45a9649f53cd0981657ee6eb268e8b for an illustration of some of these cases.) + +====================================== + ESMF initialization and finalization +====================================== + +Some unit tests require ESMF to be initialized. It is an error to re-initialize ESMF after finalizing it, so we ensure that initialization and finalization are only done once. + +To ensure that ESMF initialization is only done once, any call to ``ESMF_Initialize`` is within a conditional on the return value of ``ESMF_IsInitialized``. (ESMF appears to allow multiple initializations, with subsequent initializations not doing anything, but this behavior may not be guaranteed in the future, so to be safe, we ensure that initialization is only done once in unit testing.) + +Handling ESMF finalization is trickier, because we only want to do it at the end of a given pFUnit executable. (In practice, it might be okay to skip ESMF finalization, but this might cause issues like un-flushed log files.) To accomplish this, we use the ``EXTRA_FINALIZE`` argument to ``add_pfunit_ctest`` in the ``CMakeLists.txt`` file for any unit test that might initialize ESMF (as indicated by including ``esmf`` in the ``LINK_LIBRARIES``. (e.g., see https://github.com/ESCOMP/CTSM/pull/3017/commits/55b373b184ac56c4c1f9b837f8556cfc9ef33339.) This argument specifies a subroutine that pFUnit calls after the last test in a given executable. The associated ``EXTRA_USE`` argument gives the module name in which this subroutine is defined. Note that the ``ESMF_Finalize`` call is only done if ``ESMF_IsInitialized`` returns ``.true.``, so it's safe to use this in situations where ESMF may or may not have been initialized. + +We could handle ESMF initialization in the same way as finalization. But this might lead to unnecessary ``ESMF_Initialize`` calls. So, currently, we invoke ``ESMF_Initialize`` from the unit tests that need it, so that it's only done when truly needed by a unit test. \ No newline at end of file diff --git a/src/biogeochem/test/CNPhenology_test/CMakeLists.txt b/src/biogeochem/test/CNPhenology_test/CMakeLists.txt index 283e089ba6..c94f1502df 100644 --- a/src/biogeochem/test/CNPhenology_test/CMakeLists.txt +++ b/src/biogeochem/test/CNPhenology_test/CMakeLists.txt @@ -3,4 +3,6 @@ set (pfunit_sources add_pfunit_ctest(CNPhenology TEST_SOURCES "${pfunit_sources}" - LINK_LIBRARIES clm csm_share esmf) + LINK_LIBRARIES clm csm_share esmf + EXTRA_FINALIZE unittest_finalize_esmf + EXTRA_USE unittestInitializeAndFinalize) diff --git a/src/biogeochem/test/CNVegComputeSeed_test/CMakeLists.txt b/src/biogeochem/test/CNVegComputeSeed_test/CMakeLists.txt index b958439031..3e15ab2bed 100644 --- a/src/biogeochem/test/CNVegComputeSeed_test/CMakeLists.txt +++ b/src/biogeochem/test/CNVegComputeSeed_test/CMakeLists.txt @@ -3,4 +3,6 @@ set (pfunit_sources add_pfunit_ctest(CNVegComputeSeed TEST_SOURCES "${pfunit_sources}" - LINK_LIBRARIES clm csm_share esmf) + LINK_LIBRARIES clm csm_share esmf + EXTRA_FINALIZE unittest_finalize_esmf + EXTRA_USE unittestInitializeAndFinalize) diff --git a/src/biogeochem/test/DustEmis_test/CMakeLists.txt b/src/biogeochem/test/DustEmis_test/CMakeLists.txt index d312b721b9..88570923b8 100644 --- a/src/biogeochem/test/DustEmis_test/CMakeLists.txt +++ b/src/biogeochem/test/DustEmis_test/CMakeLists.txt @@ -5,4 +5,6 @@ set (pfunit_sources add_pfunit_ctest(DustEmis TEST_SOURCES "${pfunit_sources}" - LINK_LIBRARIES clm csm_share esmf) + LINK_LIBRARIES clm csm_share esmf + EXTRA_FINALIZE unittest_finalize_esmf + EXTRA_USE unittestInitializeAndFinalize) diff --git a/src/biogeochem/test/Latbaset_test/CMakeLists.txt b/src/biogeochem/test/Latbaset_test/CMakeLists.txt index d9f1c044f3..11c266bc21 100644 --- a/src/biogeochem/test/Latbaset_test/CMakeLists.txt +++ b/src/biogeochem/test/Latbaset_test/CMakeLists.txt @@ -3,4 +3,6 @@ set (pfunit_sources add_pfunit_ctest(CropTypeLatbaset TEST_SOURCES "${pfunit_sources}" - LINK_LIBRARIES clm csm_share esmf) + LINK_LIBRARIES clm csm_share esmf + EXTRA_FINALIZE unittest_finalize_esmf + EXTRA_USE unittestInitializeAndFinalize) diff --git a/src/biogeophys/test/Balance_test/CMakeLists.txt b/src/biogeophys/test/Balance_test/CMakeLists.txt index e140323124..ad6c157e4c 100644 --- a/src/biogeophys/test/Balance_test/CMakeLists.txt +++ b/src/biogeophys/test/Balance_test/CMakeLists.txt @@ -1,3 +1,5 @@ add_pfunit_ctest(balance TEST_SOURCES "test_Balance.pf" - LINK_LIBRARIES clm csm_share esmf) + LINK_LIBRARIES clm csm_share esmf + EXTRA_FINALIZE unittest_finalize_esmf + EXTRA_USE unittestInitializeAndFinalize) diff --git a/src/biogeophys/test/Daylength_test/CMakeLists.txt b/src/biogeophys/test/Daylength_test/CMakeLists.txt index bd2d6407a9..15c2543d8f 100644 --- a/src/biogeophys/test/Daylength_test/CMakeLists.txt +++ b/src/biogeophys/test/Daylength_test/CMakeLists.txt @@ -4,4 +4,6 @@ set (pfunit_sources add_pfunit_ctest(Daylength TEST_SOURCES "${pfunit_sources}" - LINK_LIBRARIES clm csm_share esmf) + LINK_LIBRARIES clm csm_share esmf + EXTRA_FINALIZE unittest_finalize_esmf + EXTRA_USE unittestInitializeAndFinalize) diff --git a/src/biogeophys/test/HillslopeHydrology_test/CMakeLists.txt b/src/biogeophys/test/HillslopeHydrology_test/CMakeLists.txt index 078934cc78..7573c0ce59 100644 --- a/src/biogeophys/test/HillslopeHydrology_test/CMakeLists.txt +++ b/src/biogeophys/test/HillslopeHydrology_test/CMakeLists.txt @@ -3,4 +3,6 @@ set (pfunit_sources add_pfunit_ctest(HillslopeHydrologyUtils TEST_SOURCES "${pfunit_sources}" - LINK_LIBRARIES clm csm_share esmf) + LINK_LIBRARIES clm csm_share esmf + EXTRA_FINALIZE unittest_finalize_esmf + EXTRA_USE unittestInitializeAndFinalize) diff --git a/src/biogeophys/test/Irrigation_test/CMakeLists.txt b/src/biogeophys/test/Irrigation_test/CMakeLists.txt index 4acf72961d..0fd9691e7f 100644 --- a/src/biogeophys/test/Irrigation_test/CMakeLists.txt +++ b/src/biogeophys/test/Irrigation_test/CMakeLists.txt @@ -3,4 +3,6 @@ set (pfunit_sources add_pfunit_ctest(irrigation TEST_SOURCES "${pfunit_sources}" - LINK_LIBRARIES clm csm_share esmf) + LINK_LIBRARIES clm csm_share esmf + EXTRA_FINALIZE unittest_finalize_esmf + EXTRA_USE unittestInitializeAndFinalize) diff --git a/src/biogeophys/test/Photosynthesis_test/CMakeLists.txt b/src/biogeophys/test/Photosynthesis_test/CMakeLists.txt index 628a98994a..457e62b013 100644 --- a/src/biogeophys/test/Photosynthesis_test/CMakeLists.txt +++ b/src/biogeophys/test/Photosynthesis_test/CMakeLists.txt @@ -3,4 +3,6 @@ set (pfunit_sources add_pfunit_ctest(Photosynthesis TEST_SOURCES "${pfunit_sources}" - LINK_LIBRARIES clm csm_share esmf) + LINK_LIBRARIES clm csm_share esmf + EXTRA_FINALIZE unittest_finalize_esmf + EXTRA_USE unittestInitializeAndFinalize) diff --git a/src/biogeophys/test/SnowHydrology_test/CMakeLists.txt b/src/biogeophys/test/SnowHydrology_test/CMakeLists.txt index 9e1738ab83..bfa8edc25e 100644 --- a/src/biogeophys/test/SnowHydrology_test/CMakeLists.txt +++ b/src/biogeophys/test/SnowHydrology_test/CMakeLists.txt @@ -5,4 +5,6 @@ set (pfunit_sources add_pfunit_ctest(SnowHydrology TEST_SOURCES "${pfunit_sources}" - LINK_LIBRARIES clm csm_share esmf) + LINK_LIBRARIES clm csm_share esmf + EXTRA_FINALIZE unittest_finalize_esmf + EXTRA_USE unittestInitializeAndFinalize) diff --git a/src/biogeophys/test/TotalWaterAndHeat_test/CMakeLists.txt b/src/biogeophys/test/TotalWaterAndHeat_test/CMakeLists.txt index 424515e414..d995b0a431 100644 --- a/src/biogeophys/test/TotalWaterAndHeat_test/CMakeLists.txt +++ b/src/biogeophys/test/TotalWaterAndHeat_test/CMakeLists.txt @@ -3,4 +3,6 @@ set (pfunit_sources add_pfunit_ctest(total_water_and_heat TEST_SOURCES "${pfunit_sources}" - LINK_LIBRARIES clm csm_share esmf) + LINK_LIBRARIES clm csm_share esmf + EXTRA_FINALIZE unittest_finalize_esmf + EXTRA_USE unittestInitializeAndFinalize) diff --git a/src/biogeophys/test/WaterTracerContainerType_test/CMakeLists.txt b/src/biogeophys/test/WaterTracerContainerType_test/CMakeLists.txt index 645972aa1e..9a30b674dd 100644 --- a/src/biogeophys/test/WaterTracerContainerType_test/CMakeLists.txt +++ b/src/biogeophys/test/WaterTracerContainerType_test/CMakeLists.txt @@ -3,4 +3,6 @@ set (pfunit_sources add_pfunit_ctest(water_tracer_container TEST_SOURCES "${pfunit_sources}" - LINK_LIBRARIES clm csm_share esmf) + LINK_LIBRARIES clm csm_share esmf + EXTRA_FINALIZE unittest_finalize_esmf + EXTRA_USE unittestInitializeAndFinalize) diff --git a/src/biogeophys/test/WaterTracerUtils_test/CMakeLists.txt b/src/biogeophys/test/WaterTracerUtils_test/CMakeLists.txt index 1a65bbfadd..eb259cb421 100644 --- a/src/biogeophys/test/WaterTracerUtils_test/CMakeLists.txt +++ b/src/biogeophys/test/WaterTracerUtils_test/CMakeLists.txt @@ -5,4 +5,6 @@ set (pfunit_sources add_pfunit_ctest(water_tracer_utils TEST_SOURCES "${pfunit_sources}" - LINK_LIBRARIES clm csm_share esmf) + LINK_LIBRARIES clm csm_share esmf + EXTRA_FINALIZE unittest_finalize_esmf + EXTRA_USE unittestInitializeAndFinalize) diff --git a/src/biogeophys/test/WaterType_test/CMakeLists.txt b/src/biogeophys/test/WaterType_test/CMakeLists.txt index 3f0ab409da..f1e77198ed 100644 --- a/src/biogeophys/test/WaterType_test/CMakeLists.txt +++ b/src/biogeophys/test/WaterType_test/CMakeLists.txt @@ -3,4 +3,6 @@ set (pfunit_sources add_pfunit_ctest(water_type TEST_SOURCES "${pfunit_sources}" - LINK_LIBRARIES clm csm_share esmf) + LINK_LIBRARIES clm csm_share esmf + EXTRA_FINALIZE unittest_finalize_esmf + EXTRA_USE unittestInitializeAndFinalize) diff --git a/src/biogeophys/test/Wateratm2lnd_test/CMakeLists.txt b/src/biogeophys/test/Wateratm2lnd_test/CMakeLists.txt index 1ddb840431..1aa11a2727 100644 --- a/src/biogeophys/test/Wateratm2lnd_test/CMakeLists.txt +++ b/src/biogeophys/test/Wateratm2lnd_test/CMakeLists.txt @@ -3,4 +3,6 @@ set (pfunit_sources add_pfunit_ctest(water_atm2lnd TEST_SOURCES "${pfunit_sources}" - LINK_LIBRARIES clm csm_share esmf) + LINK_LIBRARIES clm csm_share esmf + EXTRA_FINALIZE unittest_finalize_esmf + EXTRA_USE unittestInitializeAndFinalize) diff --git a/src/dyn_subgrid/test/dynConsBiogeophys_test/CMakeLists.txt b/src/dyn_subgrid/test/dynConsBiogeophys_test/CMakeLists.txt index da9c27090c..8a1fd5eb76 100644 --- a/src/dyn_subgrid/test/dynConsBiogeophys_test/CMakeLists.txt +++ b/src/dyn_subgrid/test/dynConsBiogeophys_test/CMakeLists.txt @@ -3,4 +3,6 @@ set(pfunit_sources add_pfunit_ctest(dynConsBiogeophys TEST_SOURCES "${pfunit_sources}" - LINK_LIBRARIES clm csm_share esmf) + LINK_LIBRARIES clm csm_share esmf + EXTRA_FINALIZE unittest_finalize_esmf + EXTRA_USE unittestInitializeAndFinalize) diff --git a/src/dyn_subgrid/test/dynInitColumns_test/CMakeLists.txt b/src/dyn_subgrid/test/dynInitColumns_test/CMakeLists.txt index 0adbd696ad..d6daa2a77c 100644 --- a/src/dyn_subgrid/test/dynInitColumns_test/CMakeLists.txt +++ b/src/dyn_subgrid/test/dynInitColumns_test/CMakeLists.txt @@ -1,3 +1,5 @@ add_pfunit_ctest(dynInitColumns TEST_SOURCES "test_init_columns.pf" - LINK_LIBRARIES clm csm_share esmf) + LINK_LIBRARIES clm csm_share esmf + EXTRA_FINALIZE unittest_finalize_esmf + EXTRA_USE unittestInitializeAndFinalize) diff --git a/src/dyn_subgrid/test/dynTimeInfo_test/CMakeLists.txt b/src/dyn_subgrid/test/dynTimeInfo_test/CMakeLists.txt index 625ddae91b..c8cda773ab 100644 --- a/src/dyn_subgrid/test/dynTimeInfo_test/CMakeLists.txt +++ b/src/dyn_subgrid/test/dynTimeInfo_test/CMakeLists.txt @@ -1,3 +1,5 @@ add_pfunit_ctest(dynTimeInfo TEST_SOURCES "test_dynTimeInfo.pf" - LINK_LIBRARIES clm csm_share esmf) + LINK_LIBRARIES clm csm_share esmf + EXTRA_FINALIZE unittest_finalize_esmf + EXTRA_USE unittestInitializeAndFinalize) diff --git a/src/dyn_subgrid/test/dynTimeInfo_test/test_dynTimeInfo.pf b/src/dyn_subgrid/test/dynTimeInfo_test/test_dynTimeInfo.pf index 3c6092381e..d04c68d89f 100644 --- a/src/dyn_subgrid/test/dynTimeInfo_test/test_dynTimeInfo.pf +++ b/src/dyn_subgrid/test/dynTimeInfo_test/test_dynTimeInfo.pf @@ -30,8 +30,9 @@ contains ! Make sure the date is set to the start of the year (such that the year differs ! between the start and end of the timestep), to make sure that the appropriate - ! year_position is being used. - call set_date(yr=1, mon=1, day=1, tod=0) + ! year_position is being used. (Need to use yr=2 instead of yr=1 because it's an error + ! to try to set yr,mon,day,tod = 1,1,1,0.) + call set_date(yr=2, mon=1, day=1, tod=0) end subroutine setUp subroutine tearDown(this) diff --git a/src/dyn_subgrid/test/dynVar_test/CMakeLists.txt b/src/dyn_subgrid/test/dynVar_test/CMakeLists.txt index 7164947f1e..4325c787cb 100644 --- a/src/dyn_subgrid/test/dynVar_test/CMakeLists.txt +++ b/src/dyn_subgrid/test/dynVar_test/CMakeLists.txt @@ -9,4 +9,6 @@ set (extra_sources add_pfunit_ctest(dynVar TEST_SOURCES "${pfunit_sources}" OTHER_SOURCES "${extra_sources}" - LINK_LIBRARIES clm csm_share esmf) + LINK_LIBRARIES clm csm_share esmf + EXTRA_FINALIZE unittest_finalize_esmf + EXTRA_USE unittestInitializeAndFinalize) diff --git a/src/main/test/accumul_test/CMakeLists.txt b/src/main/test/accumul_test/CMakeLists.txt index 8d7cf69f1c..3c138e3258 100644 --- a/src/main/test/accumul_test/CMakeLists.txt +++ b/src/main/test/accumul_test/CMakeLists.txt @@ -3,4 +3,6 @@ set(pfunit_sources add_pfunit_ctest(accumul TEST_SOURCES "${pfunit_sources}" - LINK_LIBRARIES clm csm_share esmf) + LINK_LIBRARIES clm csm_share esmf + EXTRA_FINALIZE unittest_finalize_esmf + EXTRA_USE unittestInitializeAndFinalize) diff --git a/src/main/test/atm2lnd_test/CMakeLists.txt b/src/main/test/atm2lnd_test/CMakeLists.txt index fcc4159ce2..1ac2e3a917 100644 --- a/src/main/test/atm2lnd_test/CMakeLists.txt +++ b/src/main/test/atm2lnd_test/CMakeLists.txt @@ -4,4 +4,6 @@ set(pfunit_sources add_pfunit_ctest(atm2lnd TEST_SOURCES "${pfunit_sources}" - LINK_LIBRARIES clm csm_share esmf) + LINK_LIBRARIES clm csm_share esmf + EXTRA_FINALIZE unittest_finalize_esmf + EXTRA_USE unittestInitializeAndFinalize) diff --git a/src/main/test/initVertical_test/CMakeLists.txt b/src/main/test/initVertical_test/CMakeLists.txt index aec45772c4..bf3bf70c35 100644 --- a/src/main/test/initVertical_test/CMakeLists.txt +++ b/src/main/test/initVertical_test/CMakeLists.txt @@ -1,3 +1,5 @@ add_pfunit_ctest(initVertical TEST_SOURCES "test_initVertical.pf" - LINK_LIBRARIES clm csm_share esmf) + LINK_LIBRARIES clm csm_share esmf + EXTRA_FINALIZE unittest_finalize_esmf + EXTRA_USE unittestInitializeAndFinalize) diff --git a/src/self_tests/test/assertions_test/CMakeLists.txt b/src/self_tests/test/assertions_test/CMakeLists.txt index aeae976839..133e8e5f01 100644 --- a/src/self_tests/test/assertions_test/CMakeLists.txt +++ b/src/self_tests/test/assertions_test/CMakeLists.txt @@ -3,4 +3,6 @@ set (pfunit_sources add_pfunit_ctest(assertions TEST_SOURCES "${pfunit_sources}" - LINK_LIBRARIES clm csm_share esmf) + LINK_LIBRARIES clm csm_share esmf + EXTRA_FINALIZE unittest_finalize_esmf + EXTRA_USE unittestInitializeAndFinalize) diff --git a/src/soilbiogeochem/test/tillage_test/CMakeLists.txt b/src/soilbiogeochem/test/tillage_test/CMakeLists.txt index 13be9aee3e..e04eec918b 100644 --- a/src/soilbiogeochem/test/tillage_test/CMakeLists.txt +++ b/src/soilbiogeochem/test/tillage_test/CMakeLists.txt @@ -1,3 +1,5 @@ add_pfunit_ctest(tillage TEST_SOURCES "test_tillage.pf" - LINK_LIBRARIES clm csm_share esmf) + LINK_LIBRARIES clm csm_share esmf + EXTRA_FINALIZE unittest_finalize_esmf + EXTRA_USE unittestInitializeAndFinalize) diff --git a/src/unit_test_shr/CMakeLists.txt b/src/unit_test_shr/CMakeLists.txt index f6a204bd72..2ded8ebe40 100644 --- a/src/unit_test_shr/CMakeLists.txt +++ b/src/unit_test_shr/CMakeLists.txt @@ -12,6 +12,7 @@ list(APPEND clm_sources unittestDustEmisInputs.F90 unittestFilterBuilderMod.F90 unittestGlcMec.F90 + unittestInitializeAndFinalize.F90 unittestSimpleSubgridSetupsMod.F90 unittestSubgridMod.F90 unittestTimeManagerMod.F90 diff --git a/src/unit_test_shr/unittestInitializeAndFinalize.F90 b/src/unit_test_shr/unittestInitializeAndFinalize.F90 new file mode 100644 index 0000000000..4aaeb80572 --- /dev/null +++ b/src/unit_test_shr/unittestInitializeAndFinalize.F90 @@ -0,0 +1,55 @@ +module unittestInitializeAndFinalize + + ! Subroutines to do per-executable initialization and finalization (i.e., things that + ! need to happen once per executable, not once per test) + + implicit none + private + + public :: unittest_finalize_esmf ! finalization routine for any tests that might have initialized ESMF + +contains + + !----------------------------------------------------------------------- + subroutine unittest_finalize_esmf() + ! + ! !DESCRIPTION: + ! Finalization routine for any tests that might have initialized ESMF + ! + ! A good rule is: for any pFUnit test that links against esmf (i.e., includes esmf in + ! the LINK_LIBRARIES line in the add_pfunit_ctest call), this finalization routine + ! should be called. (It is safe to call this even if a test didn't initialize ESMF: + ! the finalization is done conditionally.) + ! + ! This can be called by adding the following to the add_pfunit_ctest call: + ! EXTRA_FINALIZE unittest_finalize_esmf + ! EXTRA_USE unittestInitializeAndFinalize + ! + ! !USES: + use ESMF, only : ESMF_SUCCESS, ESMF_Finalize, ESMF_IsInitialized + ! + ! !LOCAL VARIABLES: + logical :: esmf_is_initialized + integer :: rc + + character(len=*), parameter :: subname = 'unittest_finalize_esmf' + !----------------------------------------------------------------------- + + esmf_is_initialized = ESMF_IsInitialized(rc=rc) + if (rc /= ESMF_SUCCESS) then + ! Calling shr_sys_abort from this finalization routine leads to a pFUnit test + ! result of pass instead of fail. Doing a 'stop 1' leads to a failure, as desired. + print *, 'Error in ESMF_IsInitialized' + stop 1 + end if + if (esmf_is_initialized) then + print *, 'Finalizing ESMF' + call ESMF_Finalize(rc=rc) + if (rc /= ESMF_SUCCESS) then + print *, 'Error in ESMF_Finalize' + stop 1 + end if + end if + end subroutine unittest_finalize_esmf + +end module unittestInitializeAndFinalize \ No newline at end of file diff --git a/src/unit_test_shr/unittestSubgridMod.F90 b/src/unit_test_shr/unittestSubgridMod.F90 index 6a1f4d9daf..531f0b041e 100644 --- a/src/unit_test_shr/unittestSubgridMod.F90 +++ b/src/unit_test_shr/unittestSubgridMod.F90 @@ -457,8 +457,12 @@ function get_ltype_special() result(ltype) end do if (.not. found) then + ! We use a 'stop 1' here instead of shr_sys_abort because, in the context of pFUnit + ! testing, a shr_sys_abort will allow the code to continue (after raising a pFUnit + ! exception), which can lead to a cryptic error due to the return value from this + ! function being invalid, giving an array out of bounds error. print *, subname//' ERROR: cannot find a special landunit' - stop + stop 1 end if end function get_ltype_special diff --git a/src/unit_test_shr/unittestTimeManagerMod.F90 b/src/unit_test_shr/unittestTimeManagerMod.F90 index 8e1cdb3f36..e0d5fba42a 100644 --- a/src/unit_test_shr/unittestTimeManagerMod.F90 +++ b/src/unit_test_shr/unittestTimeManagerMod.F90 @@ -26,6 +26,8 @@ module unittestTimeManagerMod ! clm_time_manager. The routines in this unittest-specific file, in contrast, tend to be ! higher-level wrappers. + use shr_sys_mod, only : shr_sys_abort + implicit none private save @@ -48,7 +50,7 @@ subroutine unittest_timemgr_setup(dtime, use_gregorian_calendar) ! Should be called once for every test that uses the time manager. ! ! !USES: - use ESMF, only : ESMF_Initialize, ESMF_SUCCESS + use ESMF, only : ESMF_Initialize, ESMF_IsInitialized, ESMF_SUCCESS use clm_time_manager, only : set_timemgr_init, timemgr_init, NO_LEAP_C, GREGORIAN_C ! ! !ARGUMENTS: @@ -59,6 +61,7 @@ subroutine unittest_timemgr_setup(dtime, use_gregorian_calendar) integer :: l_dtime ! local version of dtime logical :: l_use_gregorian_calendar ! local version of use_gregorian_calendar character(len=:), allocatable :: calendar + logical :: esmf_is_initialized integer :: rc ! return code integer, parameter :: dtime_default = 1800 ! time step (seconds) @@ -68,12 +71,6 @@ subroutine unittest_timemgr_setup(dtime, use_gregorian_calendar) integer, parameter :: ref_ymd = start_ymd integer, parameter :: perpetual_ymd = start_ymd - ! Set current time to be at the start of year 1 - integer, parameter :: curr_yr = 1 - integer, parameter :: curr_mon = 1 - integer, parameter :: curr_day = 1 - integer, parameter :: curr_tod = 0 - character(len=*), parameter :: subname = 'unittest_timemgr_setup' !----------------------------------------------------------------------- @@ -89,9 +86,15 @@ subroutine unittest_timemgr_setup(dtime, use_gregorian_calendar) l_use_gregorian_calendar = .false. end if - call ESMF_Initialize(rc=rc) + esmf_is_initialized = ESMF_IsInitialized(rc=rc) if (rc /= ESMF_SUCCESS) then - stop 'Error in ESMF_Initialize' + call shr_sys_abort(subname//': Error in ESMF_IsInitialized') + end if + if (.not. esmf_is_initialized) then + call ESMF_Initialize(rc=rc) + if (rc /= ESMF_SUCCESS) then + call shr_sys_abort(subname//': Error in ESMF_Initialize') + end if end if if (l_use_gregorian_calendar) then @@ -112,12 +115,6 @@ subroutine unittest_timemgr_setup(dtime, use_gregorian_calendar) call timemgr_init() - call unittest_timemgr_set_curr_date( & - yr = curr_yr, & - mon = curr_mon, & - day = curr_day, & - tod = curr_tod) - end subroutine unittest_timemgr_setup !----------------------------------------------------------------------- @@ -127,6 +124,9 @@ subroutine unittest_timemgr_set_curr_date(yr, mon, day, tod) ! Set the current model date in the time manager. This is the time at the END of the ! time step. ! + ! Note that a side effect of this subroutine is that the time step count is + ! incremented by 1 (because of the method used by for_test_set_curr_date). + ! ! !USES: use clm_time_manager, only : for_test_set_curr_date ! @@ -219,10 +219,12 @@ subroutine unittest_timemgr_teardown call timemgr_reset() - call ESMF_Finalize(rc=rc) - if (rc /= ESMF_SUCCESS) then - stop 'Error in ESMF_Finalize' - end if + ! If this is the end of the executable, we should call + ! ESMF_Finalize. But the timemgr setup and teardown routines can + ! be called multiple times within a single unit test executable, + ! and it's an error to re-call ESMF_Initialize after calling + ! ESMF_Finalize. So for now we just won't attempt to do an + ! ESMF_Finalize. end subroutine unittest_timemgr_teardown diff --git a/src/utils/clm_time_manager.F90 b/src/utils/clm_time_manager.F90 index f606c2832b..c5a1fa759b 100644 --- a/src/utils/clm_time_manager.F90 +++ b/src/utils/clm_time_manager.F90 @@ -1973,6 +1973,9 @@ subroutine timemgr_reset() ! All unit tests that modify the time manager should call this routine in their ! teardown section. ! + ! It is safe to call this subroutine even if the time manager hasn't been initialized. + ! (In this case, this reset routine won't do anything.) + ! ! Note: we could probably get away with doing much less resetting than is currently ! done here. For example, we could simply set timemgr_set = .false., and deallocate ! anything that needs deallocation. That would provide the benefit of less @@ -2001,6 +2004,13 @@ subroutine timemgr_reset() ! could simply set to time manager instance to a new instance of the derived type. ! ------------------------------------------------------------------------ + if (.not. timemgr_set) then + ! If the time manager hasn't been initialized, then we don't need to do anything. + ! This logic makes it safe to call this reset routine even in cases where the time + ! manager hasn't been initialized. + return + end if + calendar = NO_LEAP_C dtime = uninit_int @@ -2057,10 +2067,21 @@ subroutine for_test_set_curr_date(yr, mon, day, tod) ! !DESCRIPTION: ! Sets the current date - i.e., the date at the end of the time step ! + ! This is done in a way that mimics what would happen if the clock advanced from the + ! previous time step to the specified time (so, in particular, sets the previous time + ! as if that's what happened). + ! + ! Note that, because of the method used to do this setting, it is an error to try to + ! call this with the earliest possible time (yr,mon,day,tod = 1,1,1,0): instead, it + ! needs to be called with a time at least one time step later. + ! + ! Also note that an unavoidable side-effect of this method is that the time step count + ! is incremented by 1. + ! ! *** Should only be used in unit tests!!! *** ! ! !USES: - use ESMF , only : ESMF_ClockSet + use ESMF , only : ESMF_ClockSet, ESMF_ClockAdvance ! ! !ARGUMENTS: integer, intent(in) :: yr ! year @@ -2069,19 +2090,46 @@ subroutine for_test_set_curr_date(yr, mon, day, tod) integer, intent(in) :: tod ! time of day (seconds past 0Z) ! ! !LOCAL VARIABLES: - type(ESMF_Time) :: my_time ! ESMF Time corresponding to the inputs + type(ESMF_Time) :: input_time ! ESMF Time corresponding to the inputs + type(ESMF_TimeInterval) :: interval_dtime + type(ESMF_Time) :: input_time_minus_dtime integer :: rc ! return code character(len=*), parameter :: sub = 'for_test_set_curr_date' !----------------------------------------------------------------------- - call ESMF_TimeSet(my_time, yy=yr, mm=mon, dd=day, s=tod, & + ! Rather than simply setting the clock to the specified date, we instead set it to one + ! time step before the specified date, then advance the clock by a time step. This is + ! needed so that the clock's previous time is set as if we reached the specified time + ! through a typical one-time-step advance of the clock, rather than via an arbitrary + ! jump. + + ! Because of this method of setting the clock, though, it is an error to call this + ! with yr,mon,day,tod = 1,1,1,0; catch that common error here and give a meaningful + ! error message. + if (yr == 1 .and. mon == 1 .and. day == 1 .and. tod == 0) then + call shr_sys_abort(sub//': need to use a time later than yr,mon,day,tod = 1,1,1,0') + end if + + call ESMF_TimeSet(input_time, yy=yr, mm=mon, dd=day, s=tod, & calendar=tm_cal, rc=rc) call chkrc(rc, sub//': error return from ESMF_TimeSet') + + call ESMF_TimeIntervalSet(interval_dtime, s=dtime, rc=rc) + call chkrc(rc, sub//': error return from ESMF_TimeIntervalSet') + + input_time_minus_dtime = input_time - interval_dtime - call ESMF_ClockSet(tm_clock, CurrTime=my_time, rc=rc) + call ESMF_ClockSet(tm_clock, CurrTime=input_time_minus_dtime, rc=rc) call chkrc(rc, sub//': error return from ESMF_ClockSet') + ! Note that this ClockAdvance call increments the time step count; this seems like an + ! unavoidable side effect of this technique of starting with an earlier time and + ! advancing the clock (which we do in order to get the clock's previous time set + ! correctly). + call ESMF_ClockAdvance( tm_clock, rc=rc ) + call chkrc(rc, sub//': error return from ESMF_ClockAdvance') + end subroutine for_test_set_curr_date diff --git a/src/utils/test/annual_flux_dribbler_test/CMakeLists.txt b/src/utils/test/annual_flux_dribbler_test/CMakeLists.txt index 01efdc1e50..45ca43ff1c 100644 --- a/src/utils/test/annual_flux_dribbler_test/CMakeLists.txt +++ b/src/utils/test/annual_flux_dribbler_test/CMakeLists.txt @@ -1,3 +1,5 @@ add_pfunit_ctest(annual_flux_dribbler TEST_SOURCES "test_annual_flux_dribbler.pf" - LINK_LIBRARIES clm csm_share esmf) + LINK_LIBRARIES clm csm_share esmf + EXTRA_FINALIZE unittest_finalize_esmf + EXTRA_USE unittestInitializeAndFinalize) diff --git a/src/utils/test/array_utils_test/CMakeLists.txt b/src/utils/test/array_utils_test/CMakeLists.txt index a6b36304b4..e9441079b1 100644 --- a/src/utils/test/array_utils_test/CMakeLists.txt +++ b/src/utils/test/array_utils_test/CMakeLists.txt @@ -5,4 +5,6 @@ set (pfunit_sources add_pfunit_ctest(array_utils TEST_SOURCES "${pfunit_sources}" - LINK_LIBRARIES clm csm_share esmf) + LINK_LIBRARIES clm csm_share esmf + EXTRA_FINALIZE unittest_finalize_esmf + EXTRA_USE unittestInitializeAndFinalize) diff --git a/src/utils/test/clm_time_manager_test/CMakeLists.txt b/src/utils/test/clm_time_manager_test/CMakeLists.txt index 66a62e665d..38ea3c6cee 100644 --- a/src/utils/test/clm_time_manager_test/CMakeLists.txt +++ b/src/utils/test/clm_time_manager_test/CMakeLists.txt @@ -1,3 +1,5 @@ add_pfunit_ctest(clm_time_manager TEST_SOURCES "test_clm_time_manager.pf" - LINK_LIBRARIES clm csm_share esmf) + LINK_LIBRARIES clm csm_share esmf + EXTRA_FINALIZE unittest_finalize_esmf + EXTRA_USE unittestInitializeAndFinalize) diff --git a/src/utils/test/clm_time_manager_test/test_clm_time_manager.pf b/src/utils/test/clm_time_manager_test/test_clm_time_manager.pf index d2f984aa5b..00e096c8e5 100644 --- a/src/utils/test/clm_time_manager_test/test_clm_time_manager.pf +++ b/src/utils/test/clm_time_manager_test/test_clm_time_manager.pf @@ -452,7 +452,7 @@ contains londeg = londeg + 0.1_r8 ! Start at 0 Z secs = 0 - call set_date(yr=1, mon=1, day=1, tod=secs) + call set_date(yr=2, mon=1, day=1, tod=secs) do while ( .not. is_end_curr_year() ) @assertEqual( get_local_time( londeg, starttime=0 ), get_local_timestep_time( londeg ) ) call advance_timestep() diff --git a/src/utils/test/numerics_test/CMakeLists.txt b/src/utils/test/numerics_test/CMakeLists.txt index 2e826e2018..ebc85de714 100644 --- a/src/utils/test/numerics_test/CMakeLists.txt +++ b/src/utils/test/numerics_test/CMakeLists.txt @@ -3,4 +3,6 @@ set (pfunit_sources add_pfunit_ctest(numerics TEST_SOURCES "${pfunit_sources}" - LINK_LIBRARIES clm csm_share esmf) + LINK_LIBRARIES clm csm_share esmf + EXTRA_FINALIZE unittest_finalize_esmf + EXTRA_USE unittestInitializeAndFinalize)