MOM_file_parser unit test implementation

This patch introduces new features to support unit testing of the MOM6
source code.  The patch includes two new modules (MOM_unit_testing,
MOM_file_parser_tests), two new classes (UnitTest, TestSuite), and a new
driver (unit_testing).

A UnitTest object consists of the following:

* The test subroutine
* Test name (for reporting)
* A flag indicating whether the test should fail (FATAL)
* An optional cleanup subroutine

The UnitTest objects are gathered into a TestSuite object, which
provides a batch job for running all of its tests.

The use of these features is demonstrated in a driver, unit_tests, which
runs the tests provided in the MOM_file_parser_tests module

This patch also includes changes to the ".testing" build system.

* The optional FCFLAGS_COVERAGE has been removed from the testing
  Makefile.  Instead, a new "cov" target is optionally built if one
  wants to check the coverage.  It is currently based on "symmetric".

* A new "unit" target has been added to run the unit testing driver and
  report its code coverage.

* GitHub Actions has been modified to include the unit driver test.

* The gcov output now includes branching (-b), which allows reporting of
  partial line coverage in some cases.

* codecov.io "smart" report searching has been replaced with an explicit
  setting of the root directory (-R) and *.gcda paths.

Other minor changes:

* MOM_coms include an infra-level sync function (sync_PEs) as a wrapper
  to mpp_sync (or others in the future).

.codecov.yml

@@ -9,5 +9,6 @@ coverage:
                 threshold: 100%
                 base: parent
 comment:
-    # This must be set to the number of test cases (TCs)
-    after_n_builds: 8
+    # This is set to the number of TCs, plus unit, but can be removed
+    # (i.e. set to 1) when reporting is separated from coverage.
+    after_n_builds: 9

.github/actions/testing-setup/action.yml

@@ -52,7 +52,6 @@ runs:
         echo "FCFLAGS_DEBUG=-g -O0 -Wextra -Wno-compare-reals -fbacktrace -ffpe-trap=invalid,zero,overflow -fcheck=bounds" >> config.mk
         echo "FCFLAGS_REPRO=-g -O2 -fbacktrace" >> config.mk
         echo "FCFLAGS_INIT=-finit-real=snan -finit-integer=2147483647 -finit-derived" >> config.mk
-        echo "FCFLAGS_COVERAGE=--coverage" >> config.mk
         cat config.mk
         echo "::endgroup::"
 

.github/workflows/coverage.yml

@@ -20,5 +20,14 @@ jobs:
 
     - uses: ./.github/actions/testing-setup
 
+    - name: Compile unit testing
+      run: make -j build/unit/MOM6
+
+    - name: Run unit tests
+      run: make unit.cov.upload
+
+    - name: Compile MOM6 with code coverage
+      run: make -j build/cov/MOM6
+
     - name: Run and post coverage
-      run: make run.symmetric -k -s
+      run: make run.cov -k -s

.testing/Makefile

@@ -28,7 +28,7 @@
 #   MPIRUN                  MPI job launcher (mpirun, srun, etc)
 #   DO_REPRO_TESTS          Enable production ("repro") testing equivalence
 #   DO_REGRESSION_TESTS     Enable regression tests (usually dev/gfdl)
-#   REPORT_COVERAGE         Enable code coverage and report to codecov
+#   REPORT_COVERAGE         Enable code coverage and generate reports
 #
 # Compiler configuration:
 #   CC      C compiler
@@ -82,11 +82,11 @@ export MPIFC
 FCFLAGS_DEBUG ?= -g -O0
 FCFLAGS_REPRO ?= -g -O2
 FCFLAGS_OPT ?= -g -O3 -mavx -fno-omit-frame-pointer
+FCFLAGS_COVERAGE ?= -g -O0 -fbacktrace --coverage
 FCFLAGS_INIT ?=
-FCFLAGS_COVERAGE ?=
 # Additional notes:
 # - These default values are simple, minimalist flags, supported by nearly all
-#   compilers which are comparable to GFDL's canonical DEBUG and REPRO builds.
+#   compilers, and are comparable to GFDL's canonical DEBUG and REPRO builds.
 #
 # - These flags should be configured outside of the Makefile, either with
 #   config.mk or as environment variables.
@@ -95,6 +95,7 @@ FCFLAGS_COVERAGE ?=
 #   so FCFLAGS_INIT is used to provide additional MOM6 configuration.
 
 # User-defined LDFLAGS (applied to all builds and FMS)
+LDFLAGS_COVERAGE ?= --coverage
 LDFLAGS_USER ?=
 
 # Set to `true` to require identical results from DEBUG and REPRO builds
@@ -139,6 +140,9 @@ ifeq ($(DO_PROFILE), false)
   BUILDS += opt opt_target
 endif
 
+# Unit test testing
+BUILDS += cov unit
+
 # The following variables are configured by Travis:
 #   DO_REGRESSION_TESTS: true if $(TRAVIS_PULL_REQUEST) is a PR number
 #   MOM_TARGET_SLUG: TRAVIS_REPO_SLUG
@@ -165,8 +169,6 @@ else
   TARGET_CODEBASE =
 endif
 
-
-
 # List of source files to link this Makefile's dependencies to model Makefiles
 # Assumes a depth of two, and the following extensions: F90 inc c h
 # (1): Root directory
@@ -220,10 +222,8 @@ build.prof: $(foreach b,opt opt_target,build/$(b)/MOM6)
 BUILD_TARGETS = MOM6 Makefile path_names
 .PRECIOUS: $(foreach b,$(BUILDS),$(foreach f,$(BUILD_TARGETS),build/$(b)/$(f)))
 
-# Compiler flags
 
-# Conditionally build symmetric with coverage support
-COVERAGE=$(if $(REPORT_COVERAGE),$(FCFLAGS_COVERAGE),)
+# Compiler flags
 
 # .testing dependencies
 # TODO: We should probably build TARGET with the FMS that it was configured
@@ -234,28 +234,31 @@ PATH_FMS = PATH="${PATH}:../../$(DEPS)/bin"
 
 
 # Define the build targets in terms of the traditional DEBUG/REPRO/etc labels
-SYMMETRIC_FCFLAGS := FCFLAGS="$(FCFLAGS_DEBUG) $(FCFLAGS_INIT) $(COVERAGE) $(FCFLAGS_FMS)"
+SYMMETRIC_FCFLAGS := FCFLAGS="$(FCFLAGS_DEBUG) $(FCFLAGS_INIT) $(FCFLAGS_FMS)"
 ASYMMETRIC_FCFLAGS := FCFLAGS="$(FCFLAGS_DEBUG) $(FCFLAGS_INIT) $(FCFLAGS_FMS)"
 REPRO_FCFLAGS := FCFLAGS="$(FCFLAGS_REPRO) $(FCFLAGS_FMS)"
 OPT_FCFLAGS := FCFLAGS="$(FCFLAGS_OPT) $(FCFLAGS_FMS)"
 OPENMP_FCFLAGS := FCFLAGS="$(FCFLAGS_DEBUG) $(FCFLAGS_INIT) $(FCFLAGS_FMS)"
 TARGET_FCFLAGS := FCFLAGS="$(FCFLAGS_DEBUG) $(FCFLAGS_INIT) $(FCFLAGS_FMS)"
+COV_FCFLAGS := FCFLAGS="$(FCFLAGS_COVERAGE) $(FCFLAGS_FMS)"
 
 MOM_LDFLAGS := LDFLAGS="$(LDFLAGS_FMS) $(LDFLAGS_USER)"
-SYMMETRIC_LDFLAGS := LDFLAGS="$(COVERAGE) $(LDFLAGS_FMS) $(LDFLAGS_USER)"
+COV_LDFLAGS := LDFLAGS="$(LDFLAGS_COVERAGE) $(LDFLAGS_FMS) $(LDFLAGS_USER)"
 
 
 # Environment variable configuration
-build/symmetric/Makefile: MOM_ENV=$(PATH_FMS) $(SYMMETRIC_FCFLAGS) $(SYMMETRIC_LDFLAGS)
+build/symmetric/Makefile: MOM_ENV=$(PATH_FMS) $(SYMMETRIC_FCFLAGS) $(MOM_LDFLAGS)
 build/asymmetric/Makefile: MOM_ENV=$(PATH_FMS) $(ASYMMETRIC_FCFLAGS) $(MOM_LDFLAGS)
 build/repro/Makefile: MOM_ENV=$(PATH_FMS) $(REPRO_FCFLAGS) $(MOM_LDFLAGS)
 build/openmp/Makefile: MOM_ENV=$(PATH_FMS) $(OPENMP_FCFLAGS) $(MOM_LDFLAGS)
 build/target/Makefile: MOM_ENV=$(PATH_FMS) $(TARGET_FCFLAGS) $(MOM_LDFLAGS)
 build/opt/Makefile: MOM_ENV=$(PATH_FMS) $(OPT_FCFLAGS) $(MOM_LDFLAGS)
 build/opt_target/Makefile: MOM_ENV=$(PATH_FMS) $(OPT_FCFLAGS) $(MOM_LDFLAGS)
-build/coupled/Makefile: MOM_ENV=$(PATH_FMS) $(SYMMETRIC_FCFLAGS) $(SYMMETRIC_LDFLAGS)
-build/nuopc/Makefile: MOM_ENV=$(PATH_FMS) $(SYMMETRIC_FCFLAGS) $(SYMMETRIC_LDFLAGS)
-build/mct/Makefile: MOM_ENV=$(PATH_FMS) $(SYMMETRIC_FCFLAGS) $(SYMMETRIC_LDFLAGS)
+build/coupled/Makefile: MOM_ENV=$(PATH_FMS) $(SYMMETRIC_FCFLAGS) $(MOM_LDFLAGS)
+build/nuopc/Makefile: MOM_ENV=$(PATH_FMS) $(SYMMETRIC_FCFLAGS) $(MOM_LDFLAGS)
+build/mct/Makefile: MOM_ENV=$(PATH_FMS) $(SYMMETRIC_FCFLAGS) $(MOM_LDFLAGS)
+build/cov/Makefile: MOM_ENV=$(PATH_FMS) $(COV_FCFLAGS) $(COV_LDFLAGS)
+build/unit/Makefile: MOM_ENV=$(PATH_FMS) $(COV_FCFLAGS) $(COV_LDFLAGS)
 
 # Configure script flags
 build/symmetric/Makefile: MOM_ACFLAGS=
@@ -268,6 +271,8 @@ build/opt_target/Makefile: MOM_ACFLAGS=
 build/coupled/Makefile: MOM_ACFLAGS=--with-driver=FMS_cap
 build/nuopc/Makefile: MOM_ACFLAGS=--with-driver=nuopc_cap
 build/mct/Makefile: MOM_ACFLAGS=--with-driver=mct_cap
+build/cov/Makefile: MOM_ACFLAGS=
+build/unit/Makefile: MOM_ACFLAGS=--with-driver=unit_tests
 
 # Fetch regression target source code
 build/target/Makefile: | $(TARGET_CODEBASE)
@@ -276,7 +281,7 @@ build/opt_target/Makefile: | $(TARGET_CODEBASE)
 
 # Define source code dependencies
 # NOTE: ./configure is too much, but Makefile is not enough!
-#   Ideally we would want to re-run both Makefile and mkmf, but our mkmf call
+#   Ideally we only want to re-run both Makefile and mkmf, but the mkmf call
 #   is inside ./configure, so we must re-run ./configure as well.
 $(foreach b,$(filter-out target,$(BUILDS)),build/$(b)/Makefile): $(MOM_SOURCE)
 build/target_codebase/configure: $(TARGET_SOURCE)
@@ -362,11 +367,13 @@ $(DEPS)/Makefile: ../ac/deps/Makefile
 
 
 #---
-# The following block does a non-library build of a coupled driver interface to MOM, along with everything below it.
-# This simply checks that we have not broken the ability to compile. This is not a means to build a complete coupled executable.
-# Todo: 
-# - avoid re-building FMS and MOM6 src by re-using existing object/mod files
-# - use autoconf rather than mkmf templates
+# The following block does a non-library build of a coupled driver interface to
+# MOM, along with everything below it.  This simply checks that we have not
+# broken the ability to compile.  This is not a means to build a complete
+# coupled executable.
+# TODO:
+# 	- Avoid re-building FMS and MOM6 src by re-using existing object/mod files
+# 	- Use autoconf rather than mkmf templates
 MK_TEMPLATE ?= ../../$(DEPS)/mkmf/templates/ncrc-gnu.mk
 # NUOPC driver
 build/nuopc/mom_ocean_model_nuopc.o: build/nuopc/Makefile
@@ -425,11 +432,12 @@ test.dim.$(1): $(foreach c,$(CONFIGS),$(c).dim.$(1) $(c).dim.$(1).diag)
 endef
 $(foreach d,$(DIMS),$(eval $(call TEST_DIM_RULE,$(d))))
 
-.PHONY: run.symmetric run.asymmetric run.nans run.openmp
+.PHONY: run.symmetric run.asymmetric run.nans run.openmp run.cov
 run.symmetric: $(foreach c,$(CONFIGS),work/$(c)/symmetric/ocean.stats)
 run.asymmetric: $(foreach c,$(filter-out tc3,$(CONFIGS)),$(CONFIGS),work/$(c)/asymmetric/ocean.stats)
 run.nan: $(foreach c,$(CONFIGS),work/$(c)/nan/ocean.stats)
 run.openmp: $(foreach c,$(CONFIGS),work/$(c)/openmp/ocean.stats)
+run.cov: $(foreach c,$(CONFIGS),work/$(c)/cov/ocean.stats)
 
 # Configuration test rules
 # $(1): Configuration name (tc1, tc2, &c.)
@@ -573,11 +581,11 @@ work/%/$(1)/ocean.stats work/%/$(1)/chksum_diag: build/$(2)/MOM6 $(VENV_PATH)
 	@echo -e "$(DONE): $$*.$(1); no runtime errors."
 	if [ $(3) ]; then \
 	  mkdir -p results/$$* ; \
-	  cd build/symmetric ; \
-	  gcov *.gcda > gcov.$$*.$(1).out ; \
+	  cd build/$(2) ; \
+	  gcov -b *.gcda > gcov.$$*.$(1).out ; \
 	  curl -s $(CODECOV_UPLOADER_URL) -o codecov ; \
 	  chmod +x codecov ; \
-	  ./codecov -Z -f "*.gcov" -n $$@ \
+	  ./codecov -R . -Z -f "*.gcov" -n $$@ \
 	    > codecov.$$*.$(1).out \
 	    2> codecov.$$*.$(1).err \
 	    && echo -e "${MAGENTA}Report uploaded to codecov.${RESET}"; \
@@ -603,6 +611,7 @@ $(eval $(call STAT_RULE,dim.z,symmetric,,Z_RESCALE_POWER=11,,1))
 $(eval $(call STAT_RULE,dim.q,symmetric,,Q_RESCALE_POWER=11,,1))
 $(eval $(call STAT_RULE,dim.r,symmetric,,R_RESCALE_POWER=11,,1))
 
+$(eval $(call STAT_RULE,cov,cov,$(REPORT_COVERAGE),,,1))
 
 # Generate the half-period input namelist as follows:
 #  1. Fetch DAYMAX and TIMEUNIT from MOM_input
@@ -652,7 +661,6 @@ work/%/restart/ocean.stats: build/symmetric/MOM6 $(VENV_PATH)
 
 # TODO: Restart checksum diagnostics
 
-
 #---
 # Not a true rule; only call this after `make test` to summarize test results.
 .PHONY: test.summary
@@ -679,6 +687,53 @@ test.summary:
 	fi
 
 
+#---
+# unit test
+
+.PHONY: unit.cov
+unit.cov: build/unit/MOM_new_unit_tests.gcov
+
+work/unit/std.out: build/unit/MOM6
+	if [ $(REPORT_COVERAGE) ]; then \
+	  find build/unit -name *.gcda -exec rm -f '{}' \; ; \
+	fi
+	rm -rf $(@D)
+	mkdir -p $(@D)
+	cd $(@D) \
+	  && $(TIME) $(MPIRUN) -n 1 ../../$< 2> std.err > std.out \
+	  || !( \
+	    cat std.out | tail -n 100 ; \
+	    cat std.err | tail -n 100 ; \
+	  )
+	cd $(@D) \
+	  && $(TIME) $(MPIRUN) -n 2 ../../$< 2> p2.std.err > p2.std.out \
+	  || !( \
+	    cat p2.std.out | tail -n 100 ; \
+	    cat p2.std.err | tail -n 100 ; \
+	  )
+
+build/unit/codecov:
+	mkdir -p $(@D)
+	cd $(@D) \
+	  && curl -s $(CODECOV_UPLOADER_URL) -o $(@F)
+	chmod +x $@
+
+# Use driver coverage file as a proxy for the run
+# TODO: Replace work/unit/std.out with *.gcda?
+build/unit/MOM_new_unit_tests.gcov: work/unit/std.out
+	mkdir -p $(@D)
+	cd $(@D) \
+	  && gcov -b *.gcda > gcov.unit.out
+
+# Use driver coverage file as a proxy for the run
+.PHONY: unit.cov.upload
+unit.cov.upload: build/unit/MOM_new_unit_tests.gcov build/unit/codecov
+	cd build/unit \
+	  && ./codecov -R . -Z -f "*.gcov" -n "Unit tests" \
+	    > codecov.unit.out \
+	    2> codecov.unit.err \
+	  && echo -e "${MAGENTA}Report uploaded to codecov.${RESET}"
+
 #---
 # Profiling
 # XXX: This is experimental work to track, log, and report changes in runtime

ac/configure.ac

@@ -51,7 +51,11 @@ AS_IF([test "$enable_asymmetric" = yes],
 # Default to solo_driver
 DRIVER_DIR=${srcdir}/config_src/drivers/solo_driver
 AC_ARG_WITH([driver],
-  AS_HELP_STRING([--with-driver=coupled_driver|solo_driver], [Select directory for driver source code]))
+  AS_HELP_STRING(
+    [--with-driver=coupled_driver|solo_driver|unit_tests],
+    [Select directory for driver source code]
+  )
+)
 AS_IF([test "x$with_driver" != "x"],
   [DRIVER_DIR=${srcdir}/config_src/drivers/${with_driver}])
 

config_src/drivers/unit_tests/MOM_unit_test_driver.F90

@@ -0,0 +1,65 @@
+program MOM_unit_tests
+
+use MPI
+use MOM_domains, only : MOM_infra_init
+use MOM_domains, only : MOM_infra_end
+use MOM_file_parser_tests, only : run_file_parser_tests
+
+implicit none
+
+integer, parameter :: comm = MPI_COMM_WORLD
+integer, parameter :: root = 0
+integer :: rank
+logical :: file_exists_on_rank
+logical :: input_nml_exists, MOM_input_exists
+integer :: io_unit
+logical :: is_open, is_file
+integer :: rc
+
+! NOTE: Bootstrapping requires external MPI configuration.
+!   - FMS initialization requires the presence of input.nml
+!   - MOM initialization requires MOM_input (if unspecificed by input.nml)
+!   - Any MPI-based I/O prior to MOM and FMS init will MPI initialization
+! Thus, we need to do some minimal MPI setup.
+call MPI_Init(rc)
+call MPI_Comm_rank(comm, rank, rc)
+
+inquire(file='input.nml', exist=file_exists_on_rank)
+call MPI_Reduce(file_exists_on_rank, input_nml_exists, 1, MPI_LOGICAL, &
+                MPI_LOR, root, comm, rc)
+
+inquire(file='MOM_input', exist=file_exists_on_rank)
+call MPI_Reduce(file_exists_on_rank, MOM_input_exists, 1, MPI_LOGICAL, &
+                MPI_LOR, root, comm, rc)
+
+if (rank == root) then
+  ! Abort if at least one rank sees either input.nml or MOM_input
+  if (input_nml_exists) error stop "Remove existing 'input.nml' file."
+  if (MOM_input_exists) error stop "Remove existing 'MOM_input' file."
+
+  ! Otherwise, create the (empty) files
+  open(newunit=io_unit, file='input.nml', status='replace')
+  write(io_unit, '(a)') "&fms2_io_nml /"
+  close(io_unit)
+
+  open(newunit=io_unit, file='MOM_input', status='replace')
+  close(io_unit)
+endif
+
+call MOM_infra_init(comm)
+
+! Run tests
+call run_file_parser_tests
+
+! Cleanup
+call MOM_infra_end
+
+if (rank == root) then
+  open(newunit=io_unit, file='MOM_input')
+  close(io_unit, status='delete')
+
+  open(newunit=io_unit, file='input.nml')
+  close(io_unit, status='delete')
+endif
+
+end program MOM_unit_tests

config_src/infra/FMS1/MOM_coms_infra.F90

@@ -14,7 +14,7 @@ module MOM_coms_infra
 
 implicit none ; private
 
-public :: PE_here, root_PE, num_PEs, set_rootPE, Set_PElist, Get_PElist
+public :: PE_here, root_PE, num_PEs, set_rootPE, Set_PElist, Get_PElist, sync_PEs
 public :: broadcast, sum_across_PEs, min_across_PEs, max_across_PEs
 public :: any_across_PEs, all_across_PEs
 public :: field_chksum, MOM_infra_init, MOM_infra_end
@@ -108,6 +108,13 @@ subroutine Get_PEList(pelist, name, commID)
   call mpp_get_current_pelist(pelist, name, commiD)
 end subroutine Get_PEList
 
+!> Sync the PEs at a defined point in the model
+subroutine sync_PEs(pelist)
+  integer, optional, intent(in) :: pelist(:)  !< The list of PEs to be synced
+
+  call mpp_sync(pelist)
+end subroutine sync_PEs
+
 !> Communicate a 1-D array of character strings from one PE to others
 subroutine broadcast_char(dat, length, from_PE, PElist, blocking)
   character(len=*),  intent(inout) :: dat(:)    !< The data to communicate and destination

config_src/infra/FMS2/MOM_coms_infra.F90

@@ -14,7 +14,7 @@ module MOM_coms_infra
 
 implicit none ; private
 
-public :: PE_here, root_PE, num_PEs, set_rootPE, Set_PElist, Get_PElist
+public :: PE_here, root_PE, num_PEs, set_rootPE, Set_PElist, Get_PElist, sync_PEs
 public :: broadcast, sum_across_PEs, min_across_PEs, max_across_PEs
 public :: any_across_PEs, all_across_PEs
 public :: field_chksum, MOM_infra_init, MOM_infra_end
@@ -108,6 +108,13 @@ subroutine Get_PEList(pelist, name, commID)
   call mpp_get_current_pelist(pelist, name, commiD)
 end subroutine Get_PEList
 
+!> Sync the PEs at a defined point in the model
+subroutine sync_PEs(pelist)
+  integer, optional, intent(in) :: pelist(:)  !< The list of PEs to be synced
+
+  call mpp_sync(pelist)
+end subroutine sync_PEs
+
 !> Communicate a 1-D array of character strings from one PE to others
 subroutine broadcast_char(dat, length, from_PE, PElist, blocking)
   character(len=*),  intent(inout) :: dat(:)    !< The data to communicate and destination