Avoid repeated ESMF initialization in unittestTimeManagerMod.F90

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.

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)

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)

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)

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)

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)

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)

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)

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)

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)

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)

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)

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)

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)

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)

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)

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)

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)

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)

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)

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)

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)

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)

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)

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)

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)

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

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

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