Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add unit test for existing point output I/O code #1158

Merged
merged 25 commits into from
Jan 4, 2024

Conversation

edwardhartnett
Copy link
Contributor

Pull Request Summary

This PR adds a unit test for the existing points I/O code.

Description

Unit test is added. Also turned on code coverage in the CI.

Please also include the following information:
Once this is merged we can talk about how we want the other CI workflows to run unit tests.

Issue(s) addressed

Part of #682

Commit Message

Add unit test for points I/O code.

Check list

Testing

  • How were these changes tested?

No code change in this PR, only tests being added for existing code.

This PR includes the contents of #1157

@edwardhartnett edwardhartnett marked this pull request as ready for review December 26, 2023 20:40
@MatthewMasarik-NOAA
Copy link
Collaborator

Thanks @edwardhartnett. I'll start reviewing this.

@MatthewMasarik-NOAA
Copy link
Collaborator

Timeline for processing, tomorrow (1/3).

@edwardhartnett
Copy link
Contributor Author

Once this is merged we can turn on unit tests in the other CI runs. We can also discuss the proliferation of CI workflows and how they might best be organized.

@JessicaMeixner-NOAA
Copy link
Collaborator

What are thoughts about moving the model/test directory to under the regression test directory and perhaps renaming it to unit tests so it'd be regtests/unit or regtests/unittests or something similar? I think this is more in line with the current WW3 directory structure.

@edwardhartnett
Copy link
Contributor Author

I'm happy to move them. @MatthewMasarik-NOAA you agree?

@MatthewMasarik-NOAA
Copy link
Collaborator

Hi @edwardhartnett @JessicaMeixner-NOAA, yes that works for me. My vote would be for: regtests/unittests.

@edwardhartnett
Copy link
Contributor Author

OK I've moved the test directory as suggested.

@MatthewMasarik-NOAA
Copy link
Collaborator

OK I've moved the test directory as suggested.

Great! Thank you @edwardhartnett, the directory structure looks good.

@MatthewMasarik-NOAA
Copy link
Collaborator

@edwardhartnett

Update: The CI runs on my fork of this branch just came back all successful, so we are good to proceed.

One change: Could we have the new CI test be turned off by default? I see there's an if statement controlling the unit tests with the BUILD_TESTING parameter. Could that be set to false by default, but allow you to override by passing in -DBUILD_TESTING=1? I talked with @JessicaMeixner-NOAA this morning about the Organization Free github plans and how adding CI testing would affect our allotment. The free plan gives us 2000 minutes per month, but that is for all 178 repositories in NOAA-EMC. I tried from the NOAA-EMC github home to see if I could view usage data, but could not. Since we don't know how much we're consuming, but know that we share a fixed budget with 177 other repos, it seems smart to turn this off until we have a solution with respect to allotment. Does that work?

@ukmo-ccbunney
Copy link
Collaborator

It's great to see some work towards unit testing in the WW3 code!

@edwardhartnett
Copy link
Contributor Author

I will turn off GitHub Actions unit testing in this PR, as requested.

@ukmo-ccbunney I agree it would be great to see some unit testing of this code. Programmers work an order-of-magnitude faster on code bases with good unit testing. Nor am I comfortable handing over code to a project that may break it, all unknowing. I like my software to be reliable.

I gave a talk about unit testing NetCDF and NCEPLIBS at the Model Correctness Workshop recently at NCAR. We also presented an AGU poster about adding unit testing to NCEPLIBS.

Adding unit testing to science codes is straightforward - easy programming, in fact. The WW3 code base contains about 260K model code (at least half of which is comments). So I would expect writing full tests would take a good programmer less than a year. In the absence of a dedicated testing programmer, that means if every WW3 programmer wrote tests for all new code, and also pitched in to write tests for some existing code, then WW3 would have unit testing in less than a year.

There must be top-down stakeholder support for unit testing. Without full support from project leadership and senior developers, any such effort will be unsuccessful. Unit testing is a discipline which requires effort and commitment from all programmers and leadership.

@MatthewMasarik-NOAA to further pursue unit testing for WW3, if GitHub is disallowed, we can talk about setting up a Jenkins server on your machine which would run the test code.

Jenkins:

  • Free and open source java-based CI system.
  • Really nice and easy GUI. Much better than GitHub yml files.
  • Can be set up on any system.
  • Can be integrated with GitHub with some extra work and some extra tweaks from sysadmins.
  • Can run much bigger tests, MPI tests, tests that need a lot of data; it's limited only by your own hardware.
  • Does not have to download data or install dependencies for every test run, so much faster.

Every programmer on WW3 should do as I have done: start adding tests for the tasks you are working on. Without programmer effort, this code will never get tests. If neither GitHub nor Jenkins were available, there are still other CI solutions.

The determined programmer can have CI on any project, and the wise programmer is determined to use CI on every project.

@edwardhartnett
Copy link
Contributor Author

I just installed jenkins on my machine; it took about 10 minutes. Good instructions here: https://phoenixnap.com/kb/install-jenkins-ubuntu#:~:text=installed%20on%20Ubuntu.-,Step%201%3A%20Install%20Java,and%20Java%2011%20on%20Ubuntu.

@MatthewMasarik-NOAA
Copy link
Collaborator

I will turn off GitHub Actions unit testing in this PR, as requested.

@ukmo-ccbunney I agree it would be great to see some unit testing of this code. Programmers work an order-of-magnitude faster on code bases with good unit testing. Nor am I comfortable handing over code to a project that may break it, all unknowing. I like my software to be reliable.

I gave a talk about unit testing NetCDF and NCEPLIBS at the Model Correctness Workshop recently at NCAR. We also presented an AGU poster about adding unit testing to NCEPLIBS.

Adding unit testing to science codes is straightforward - easy programming, in fact. The WW3 code base contains about 260K model code (at least half of which is comments). So I would expect writing full tests would take a good programmer less than a year. In the absence of a dedicated testing programmer, that means if every WW3 programmer wrote tests for all new code, and also pitched in to write tests for some existing code, then WW3 would have unit testing in less than a year.

There must be top-down stakeholder support for unit testing. Without full support from project leadership and senior developers, any such effort will be unsuccessful. Unit testing is a discipline which requires effort and commitment from all programmers and leadership.

@MatthewMasarik-NOAA to further pursue unit testing for WW3, if GitHub is disallowed, we can talk about setting up a Jenkins server on your machine which would run the test code.

Jenkins:

* Free and open source java-based CI system.

* Really nice and easy GUI. Much better than GitHub yml files.

* Can be set up on any system.

* Can be integrated with GitHub with some extra work and some extra tweaks from sysadmins.

* Can run much bigger tests, MPI tests, tests that need a lot of data; it's limited only by your own hardware.

* Does not have to download data or install dependencies for every test run, so much faster.

Every programmer on WW3 should do as I have done: start adding tests for the tasks you are working on. Without programmer effort, this code will never get tests. If neither GitHub nor Jenkins were available, there are still other CI solutions.

The determined programmer can have CI on any project, and the wise programmer is determined to use CI on every project.

@edwardhartnett Thank you for the insights on testing. The presentation slides are nice, and can give folks an introduction/refresher to unit testing as we go forward. @JessicaMeixner-NOAA and I have started to have some discussion on incorporating this type of testing, and we've acknowledged this will require a larger discussion for us internally to determine how to proceed.

As we determine our posture, I'm interested to talk more about running jenkins for CI. I believe we could also use a self-hosted runner under github Actions, which is free. Whatever our solution, and I agree we can find a suitable alternative to running on Github if need be, we would need to get it to run on RDHPCS. I'll look forward to talking more offline about this.

I'll include the review next.

Copy link
Collaborator

@MatthewMasarik-NOAA MatthewMasarik-NOAA left a comment

Choose a reason for hiding this comment

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

Thanks for making the change to toggle the test to off for now, @edwardhartnett. I re-ran the CI in my fork and the current Intel/GNU builds were successful, and the gnu_io test does not run, so everything functions as expected.

Code review
Pass

Testing
Pass

  • io_gnu workflow ran successfully when enabled.
  • io_gnu confirmed turned off for now (due to GH Actions quota constraints for Organizations).

@MatthewMasarik-NOAA MatthewMasarik-NOAA merged commit 7bec560 into NOAA-EMC:develop Jan 4, 2024
20 checks passed
@MatthewMasarik-NOAA
Copy link
Collaborator

Thanks @edwardhartnett. It's great to see this first checkpoint for the point I/O work, and additionally a first step in the direction of increasing our testing footprint.

@edwardhartnett edwardhartnett deleted the ejh_unit_test branch April 5, 2024 10:57
miguelsolanocordoba added a commit to wavespotter/WW3 that referenced this pull request Apr 19, 2024
* Bugfix - initialised VD and VS to zero in w3srcemd. (NOAA-EMC#1037)

* More efficient test for binary files in matrix.comp (NOAA-EMC#1035)

* Tidy up of pre-processor directives and unused variables in w3srcemd.F90 (NOAA-EMC#1010)

* Correct typo in w3srcemd.F90 pre-processor directive. (NOAA-EMC#1039)

* minor bugfix for matrix grepping on keywords (NOAA-EMC#1049)

* Stop masking group 1 output where icec > icen (NOAA-EMC#1019)

* Doxygen documentation added, 8th subset.(NOAA-EMC#1046)

* NC4 ,F90 ,XX0 switches removed from ww3_tp2.19 regtest (NOAA-EMC#1054)

* CI:  Fix for Intel scripts. GNU scripts updated. (NOAA-EMC#1064)

* correct the computation of QP parameter, add QKK output parameter, change UST scale factor (NOAA-EMC#1050)

* correct issue with ww3_multi when requesting restart2 and using nml file instead of inp file (NOAA-EMC#1070)

* correct calendar for track netcdf output (NOAA-EMC#1079)

* Fix missing mod_def.ww3 file in multigrid regression tests for track output (NOAA-EMC#1091)

* STAB3: fix cmake build for ST4 or ST3 (NOAA-EMC#1086)

* new feature to output out_grd.ww3, out_pnt.ww3 and mod_def.ww3 both in binary and ascii format using switch ASCII. (NOAA-EMC#1089)

* Update local unit number arrays (NDS, MDS) to be same size of array defined in w3odatmd (size=15). Also, defined unit numbers for NDS(14) and NDS(15). (NOAA-EMC#1098)

* Removed code referencing PHIOC in output section for PHICE in ww3_ounf (NOAA-EMC#1093)

* implementation of the GQM (Gaussian Quadrature Method) to replace the DIA in NL1 or NL2. (NOAA-EMC#1083)

* update logic to ensure you are not accessing uninitialized dates (NOAA-EMC#1114)

* Initialised S and D arrays in W3SDB1 before potential early return if zero energy. (NOAA-EMC#1115)

* ww3_ounp.F90:  x/y units attribute corrected from 'm' to 'km' (NOAA-EMC#1088)

* Bugfix: Assign unit numbers to ASCII gridded/point output in multi-grid mode. (NOAA-EMC#1118)

* correct bugs to run correctly GQM implementation (NOAA-EMC#1127)

* Adding documentation to w3iopo() in preparation for code for NOAA-EMC#682. (NOAA-EMC#1131)

* NCEP regtest module updates: uses spack-stack/1.5.0, includes scotch/7.0.4 (NOAA-EMC#1137)

* Minor update to ncep regtests (NOAA-EMC#1138)

* Updated intel workflow to install oneapi compilers from new location. (NOAA-EMC#1157)

* Add unit test for points I/O code. (NOAA-EMC#1158)

* Update Intel CI (relocate /usr/local; ensure intel-oneapi-mpi; use ubuntu-latest) (NOAA-EMC#1161)

* remove lookup table for ST4 to speed up computation and clean up the ST4 code (NOAA-EMC#1124)

Co-authored-by: Fabrice Ardhuin <[email protected]>

* initialize USSP_WN for mod_def (NOAA-EMC#1165)

* Introduce IC4M8 and IC4M9 to WW3 (NOAA-EMC#1176)

* clean up and add ST4 variables (NOAA-EMC#1181)

* w3fld1md.F90: fix divide by zero in CRIT2 parameter (NOAA-EMC#1184)

* ww3_prnc.F90: fix out-of-scope grid index write statement (NOAA-EMC#1185)

* Bugfix: address potential divide-by-zero in APPENDTAIL (NOAA-EMC#1188)

Co-authored-by: Denise Worthen <[email protected]>

* Provide initial drying of cells with depth < ZLIM for SMC grid. (NOAA-EMC#1192)

* Output OMP threading info to screen when running ww3_shel/ww3_multi compiled with the OMPG switch. Also fixes truncation of build.log when running run_cmake_build. (NOAA-EMC#1191)

* Added screen output showing number of threads when OMP enabled.

* update build to get more info in logs (NOAA-EMC#46)

---------

Co-authored-by: Jessica Meixner <[email protected]>

* update run_cmake_test to catch build errors and exit (NOAA-EMC#1194)

* fix merge conflicts

* Fix gustiness bug, as suggst by Pieter

* Change USTARsigma to WAM implementation

---------

Co-authored-by: Chris Bunney <[email protected]>
Co-authored-by: Mickael Accensi <[email protected]>
Co-authored-by: Benoit Pouliot <[email protected]>
Co-authored-by: Matthew Masarik <[email protected]>
Co-authored-by: Ghazal-Mohammadpour <[email protected]>
Co-authored-by: Jessica Meixner <[email protected]>
Co-authored-by: Biao Zhao <[email protected]>
Co-authored-by: Edward Hartnett <[email protected]>
Co-authored-by: Alex Richert <[email protected]>
Co-authored-by: Fabrice Ardhuin <[email protected]>
Co-authored-by: W. Erick Rogers <[email protected]>
Co-authored-by: Denise Worthen <[email protected]>
Co-authored-by: Camille Teicheira <[email protected]>
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.

4 participants