-
Notifications
You must be signed in to change notification settings - Fork 543
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
Add unit test for existing point output I/O code #1158
Conversation
Thanks @edwardhartnett. I'll start reviewing this. |
Timeline for processing, tomorrow (1/3). |
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. |
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. |
I'm happy to move them. @MatthewMasarik-NOAA you agree? |
Hi @edwardhartnett @JessicaMeixner-NOAA, yes that works for me. My vote would be for: regtests/unittests. |
OK I've moved the test directory as suggested. |
Great! Thank you @edwardhartnett, the directory structure looks good. |
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 |
It's great to see some work towards unit testing in the WW3 code! |
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:
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. |
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. |
@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. |
There was a problem hiding this 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).
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. |
* 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]>
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
No code change in this PR, only tests being added for existing code.
This PR includes the contents of #1157