Adds test for mpp_init_logfile #510

ngs333 · 2020-07-10T23:09:28Z

Description
This PR creates a unit test for the mpp interface mpp_init_logfile()

Fixes # (issue)

How Has This Been Tested?
On both the skylake server with intel compiler, and a home computer with gcc compiler,
libFMS and the unit test applications were compiled. The unite tests for subdirectory FMS/est_fms/mpp were run via "make check" from the command line. Additionally, the unit test brought in with this PR was made made to run as expected under various conditions and the side effects on the files were verified to be as expected.

Note and warning: Interface mpp_init_logfile() did not have any documentation. I made an interpretation on how its expected to run and commented the interface to contain that documentation. In order to test from an external program, I made the interface public. Let me know if this is an issue.

Checklist:

My code follows the style guidelines of this project
I have performed a self-review of my own code
I have commented my code, particularly in hard-to-understand areas
I have made corresponding changes to the documentation
My changes generate no new warnings
Any dependent changes have been merged and published in downstream modules
New check tests, if applicable, are included
make distcheck passes

thomas-robinson · 2020-07-13T12:36:20Z

mpp/include/mpp_util.inc

+  ! <FUNCTION NAME="mpp_init_logfile">
+  !  <OVERVIEW>
+  !    Initialize the logfile.
+  !  </OVERVIEW>
+  !  <DESCRIPTION>
+  !    This function re-initializes the log files of the PEs. Re-initialization
+  !    means replacing if it already exists. It does not make new files if they don't
+  !    already exist. Only log files with names of format "<header>.pe_number.out",
+  !    where pe_number is the digit integer represeting the PE number, such as 000000 for the
+  !    root PE, 000001 for the next PE, etch. The <header> is the config file name prefix -
+  !    a module global normally set elsewhere, and frequetly is the integer simulation
+  !    timedate (e.g. 19480101).
+  !  </DESCRIPTION>
+  ! </FUNCTION>
+


Any added documentation should be in doxygen format. Please reformat

I think I have reformatted now!

thomas-robinson · 2020-07-13T12:37:16Z

test_fms/mpp/test_mpp_init_logfile.F90

+!! @email gfdl.climate.model.info@noaa.gov
+
+program test_mpp_init_logfile
+#include <fms_platform.h>


You should use platforms_mod instead

Thats what I get for copy and pasting. I dont even use it.

thomas-robinson · 2020-07-13T12:38:01Z

mpp/mpp.F90

@@ -230,6 +230,9 @@ module mpp_mod
  public :: read_ascii_file, read_input_nml, mpp_clock_begin, mpp_clock_end
  public :: get_ascii_file_num_lines
  public :: mpp_record_time_start, mpp_record_time_end
+  !! TODO:
+  public :: mpp_init_logfile
+


Remove this whitespace

I think its removed now!

thomas-robinson · 2020-07-13T12:38:32Z

mpp/mpp.F90

+  !! TODO:
+  public :: mpp_init_logfile


Are you still working on this? Are you making a private routine public?

I was hoping to not make it public. Is there a better way to get access to mpp_init_logfile from the unit test app.

This may remain a problem (with both pros and cons) see the issue below concerning mpp_init_test_logfile_init

Thanks for bringing up the issue in the meeting. Fortunately for this test is a non-issue now.

thomas-robinson · 2020-07-13T12:40:09Z

test_fms/mpp/test_mpp_init_logfile.F90

+  integer :: err_no
+
+  !Initialize mpp but have mpp_init() exit before log files are initalized.
+  my_mpp_init_level = mpp_init_test_logfile_init - 1


Why are you subtracting 1? Isn't there a mpp_init_test level that you can directly use?

I was subtracting one so that initialization stops before calling init_logfile. But given the
challenge of keeping init_logfile private, and upon inspection of the mpp_init routine, it occurs to me that I can solve both issues just by not subtracting one and not calling init_log_file.
Thanks.

I am not sure and am requesting input. If the init level is set to "mpp_init_test_logfile_init".
then mpp_init() actually does more work AFTER calling mpp_initlogfile: it writes to the logifle. So technically, to test the interface, one should isolate the interface to whatever extent possible, not after what it creates is used by another function.

Its changed so it not subtracting now. Corresponding changed in the shell script were made. Mpp_init_logfile was removed from public (i.e. put back to original form).

thomas-robinson · 2020-07-13T12:41:11Z

test_fms/mpp/test_mpp_init_logfile.F90

+  my_mpp_init_level = mpp_init_test_logfile_init - 1
+  call mpp_init( test_level = my_mpp_init_level )
+
+  call mpp_init_logfile()


is anything actually tested, or is this routine just called?

Most of the test work is actually done is done by the shell command calling this the fortran test program. The shell program creates file and checks the effects of the called app. Like many I/o routines, mpp_init_logfile() is merely a side_effect routine. So what the shell comman does is test that it makes it side effects correctly and also that it doesn't make additional side effects that it could potentially make. This means, checking the files that it should replace, and checking for the lack of existence of files that it should not make but that whose names are iterated over in the do loop of the routine. If you look at the shell command that calls it, you should be able to see that it returns a '1' when those conditions are not satisfied. (PS I checked that the CI summary shows "FAILED" is the shell command returns 1, and success if it returns 0.)

I updated the comments in both the Fortran test program and the corresponding shell script. It better explains what they are doing together.

…e comments as suggested by merge reviewers.

… test_mpp_initlog

test_fms/mpp/test_mpp_init_logfile.F90

…nit. This also caused changes to the made in the shell script and mpp.F90 (mpp_init_logfile function was removed from the public interface, i.e. back to original setting).

rem1776 · 2021-08-13T17:29:05Z

test included in #800

Adding unit test for interface mpp_init_logfile.

c50d06d

ngs333 requested review from colingladueNOAA, thomas-robinson and wrongkindofdoctor as code owners July 10, 2020 23:09

Cleaning comments in file test_mpp_init_logfile.sh

6990279

colingladueNOAA self-assigned this Jul 11, 2020

colingladueNOAA added development enhancement Issue/PR for a modification that increases performance, improves syntax, or adds functionality. labels Jul 11, 2020

Merge branch 'master' into test_mpp_initlog

aecd519

wrongkindofdoctor previously approved these changes Jul 13, 2020

View reviewed changes

thomas-robinson requested changes Jul 13, 2020

View reviewed changes

ngs333 added 2 commits July 15, 2020 17:33

Cleaned up the Fortran and shell script and and improved the main cod…

b6af028

…e comments as suggested by merge reviewers.

Merge branch 'test_mpp_initlog' of https://github.com/ngs333/FMS into…

8e5c139

… test_mpp_initlog

ngs333 dismissed wrongkindofdoctor’s stale review via 8e5c139 July 15, 2020 21:40

thomas-robinson requested changes Jul 16, 2020

View reviewed changes

test_fms/mpp/test_mpp_init_logfile.F90 Outdated Show resolved Hide resolved

Modifying tes executable to call mpp_init at level mpp_init_logfile_i…

9d251f4

…nit. This also caused changes to the made in the shell script and mpp.F90 (mpp_init_logfile function was removed from the public interface, i.e. back to original setting).

wrongkindofdoctor approved these changes Jul 16, 2020

View reviewed changes

Base automatically changed from master to main March 8, 2021 16:41

thomas-robinson requested review from bensonr and rem1776 as code owners March 8, 2021 16:41

rem1776 closed this Aug 13, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds test for mpp_init_logfile #510

Adds test for mpp_init_logfile #510

ngs333 commented Jul 10, 2020 •

edited by colingladueNOAA

Loading

thomas-robinson Jul 13, 2020

ngs333 Jul 16, 2020

thomas-robinson Jul 13, 2020

ngs333 Jul 15, 2020

thomas-robinson Jul 13, 2020

ngs333 Jul 16, 2020

thomas-robinson Jul 13, 2020

ngs333 Jul 15, 2020

ngs333 Jul 15, 2020

ngs333 Jul 16, 2020

thomas-robinson Jul 13, 2020

ngs333 Jul 15, 2020

ngs333 Jul 15, 2020

ngs333 Jul 16, 2020

thomas-robinson Jul 13, 2020

ngs333 Jul 15, 2020 •

edited

Loading

ngs333 Jul 15, 2020

rem1776 commented Aug 13, 2021

Adds test for mpp_init_logfile #510

Adds test for mpp_init_logfile #510

Conversation

ngs333 commented Jul 10, 2020 • edited by colingladueNOAA Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ngs333 Jul 15, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rem1776 commented Aug 13, 2021

ngs333 commented Jul 10, 2020 •

edited by colingladueNOAA

Loading

ngs333 Jul 15, 2020 •

edited

Loading