Skip to content

MOM_file_parser unit test implementation#119

Merged
Hallberg-NOAA merged 1 commit into
NOAA-GFDL:dev/gfdlfrom
marshallward:unittest
May 20, 2022
Merged

MOM_file_parser unit test implementation#119
Hallberg-NOAA merged 1 commit into
NOAA-GFDL:dev/gfdlfrom
marshallward:unittest

Conversation

@marshallward
Copy link
Copy Markdown
Member

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
Copy link
Copy Markdown

codecov Bot commented Apr 29, 2022

Codecov Report

Merging #119 (7990210) into dev/gfdl (9d6def6) will increase coverage by 4.71%.
The diff coverage is 69.21%.

❗ Current head 7990210 differs from pull request most recent head cf448a1. Consider uploading reports for the commit cf448a1 to get more accurate results

@@             Coverage Diff              @@
##           dev/gfdl     #119      +/-   ##
============================================
+ Coverage     28.81%   33.52%   +4.71%     
============================================
  Files           249      262      +13     
  Lines         72934    71175    -1759     
  Branches          0    13284   +13284     
============================================
+ Hits          21015    23861    +2846     
+ Misses        51919    42844    -9075     
- Partials          0     4470    +4470     
Impacted Files Coverage Δ
src/framework/MOM_coms.F90 44.57% <ø> (-11.27%) ⬇️
config_src/infra/FMS1/MOM_coms_infra.F90 34.61% <33.33%> (-8.75%) ⬇️
src/framework/testing/MOM_file_parser_tests.F90 68.58% <68.58%> (ø)
src/framework/MOM_unit_testing.F90 69.33% <69.33%> (ø)
...ig_src/drivers/unit_tests/MOM_unit_test_driver.F90 100.00% <100.00%> (ø)
src/diagnostics/MOM_obsolete_diagnostics.F90 25.00% <0.00%> (-45.59%) ⬇️
src/core/MOM_unit_tests.F90 16.66% <0.00%> (-44.45%) ⬇️
src/tracer/MOM_tracer_flow_control.F90 17.31% <0.00%> (-35.30%) ⬇️
src/core/MOM_continuity.F90 53.57% <0.00%> (-23.36%) ⬇️
src/framework/MOM_write_cputime.F90 65.07% <0.00%> (-21.08%) ⬇️
... and 235 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9d6def6...cf448a1. Read the comment docs.

Comment thread src/framework/MOM_unit_testing.F90 Outdated
use MOM_error_handler, only : disable_fatal_errors
use MOM_error_handler, only : enable_fatal_errors

implicit none
Copy link
Copy Markdown
Member

@Hallberg-NOAA Hallberg-NOAA May 10, 2022

Choose a reason for hiding this comment

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

The convention documented at https://github.com/NOAA-GFDL/MOM6/wiki/Code-style-guide is to have implicit none ; private on the same line in every file that is not a driver (i.e., has main). This help us to easily detect files that do not make these important declarations at the topmost level; otherwise the private declaration might apply to the contents of a type or something else. The same comment also applies to lines 31 and 32 of MOM_file_parser_tests.F90.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I don't understand, I am using private already.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Doing grep -L implicit none ; private would flag this file as potentially problematic if this convention is not followed.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I have added the Hallberg semicolons.

@Hallberg-NOAA
Copy link
Copy Markdown
Member

I have examined the changes to the F90 files, and apart from one minor concern, they look good to me. However, I am not well versed with .yml, .ac, or Makefile syntax, and would like to have someone else comment on those changes.

@marshallward marshallward force-pushed the unittest branch 2 times, most recently from b004668 to 9eda7fd Compare May 16, 2022 18:17
Copy link
Copy Markdown
Member

@Hallberg-NOAA Hallberg-NOAA left a comment

Choose a reason for hiding this comment

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

I have examined these changes, and despite not being fluent in the syntax of .yml, .ac, or Makefiles, all of the changes here seem sensible. I will trigger the pipeline testing of these changes after this PR comes to the top of the queue, and after updating the base branch.

@Hallberg-NOAA
Copy link
Copy Markdown
Member

The PGI build step in the pipeline is failing, apparently at lines 761 and 780 of MOM_file_parser_tests.F90. I have tried this 3 times, with the same result each time. It appears that the pgi compiler is interpreting the " /'" construct as an escape on the closing quote, leading to the erroneous error messages about mismatched quotes., @marshallward, please see what needs to be done to get these changes to compile with the pgi compiler.

Copy link
Copy Markdown
Member

@Hallberg-NOAA Hallberg-NOAA left a comment

Choose a reason for hiding this comment

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

I thought that these changes looked good, but apparently the pgi compiler disagrees. Please make the changes that are needed to get the pgi compiler to work.

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).
@marshallward
Copy link
Copy Markdown
Member Author

We should be building with PGI -Mbackslash but I have replaced \ with achar(92)

@Hallberg-NOAA
Copy link
Copy Markdown
Member

With the updates, this PR has now passed the pipeline testing at https://gitlab.gfdl.noaa.gov/ogrp/MOM6/-/pipelines/15555.

@Hallberg-NOAA Hallberg-NOAA merged commit 9292b58 into NOAA-GFDL:dev/gfdl May 20, 2022
@marshallward marshallward deleted the unittest branch June 14, 2022 19:22
marshallward pushed a commit that referenced this pull request Oct 25, 2023
update to MOM6 main 20230731 and 20230811 updating
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants