Testing refactor #612

mwaxmonsky · 2024-11-11T04:48:42Z

Refactoring of testing infrastructure

Changes include:

Refactored python tests to leverage unittest framework.
Replaced shell tests with equivalent wrapper from Python.
Reduces value duplication from fortran/shell/python files
to just fortran and python file.
Adds re-useable CMake infrastructure for capgen, advection,
ddthost, and var_compatability tests.
Adds cmake functions to wrap calls to ccpp_capgen.py and ccpp_datafile.py.
Enabling re-use of openmp and mpi configuration for both library
and tests.
Removing PGI compiler options and adding ifx support.
Replaces manually calling fortran tests through run_test with ctest
Removed a few un-used files
Adds missing dependencies and --exclude-required tests.
Removed explicit optimization flag setting and relying on default CMake behavior from CMAKE_BUILD_TYPE.
Removed -DDEBUG flag as generated code requires a --debug flag to insert debug logic explicitly.
Updated test/unit-test docs to reflect current build/run process.
Added docs documentation to demonstrate building of docs.

TODO:

~~Convert Fortran tests to pFUnit tests~~ Removing pFUnit for now.
Determine optimal layout of python direct/command line calls
~~Determine if yaml or json is possible for Fortran to have single source for variable names and integrate into workflow~~ Will implement in a separate PR to reduce review.

User interface changes?: No

Fixes: None
Resolves #514

Testing:
test removed: None
unit tests:
system tests:
manual testing:

… with same binary name.

mwaxmonsky · 2024-11-11T05:12:19Z

Greetings all, apologies for throwing this out there without an issue but I've been trying to keep up with the changes in the framework and the equivalent tests and was getting lost in the details and noticed a few things that I thought would make things a little bit easier overall.

My main goal was making it more explicit what was being tested and reducing the amount of duplication we have but that necessitated quite a few changes at various levels of the infrastructure.

I started going down a rabbit hole of how the framework is built and tested but before going much further, I figured I would share some of the things I've updated and check in to see if these make sense and are useful for the rest of the team.

There are a few technical details that need to be evaluated as well but I wanted to check if, overall, this is moving things in a reasonable direction or if this is making things more complicated?

gold2718 · 2024-11-11T08:11:03Z

Hi @mwaxmonsky, thanks for tackling this.

So far, most of this looks like much needed cleanup!

There is one change I do not understand. The old (clumsy, wordy) run_test script tested both the Python and the bash interfaces to ccpp_datafile.py. Do the new tests test the bash interface? These are (or at least were) used by some build systems.

climbfuji

There is a lot of great stuff in this PR, thanks @mwaxmonsky.

My main concern with this PR is that it touches code/build files that aren't just used by the unit tests, but also by production code (e.g. the top-level CMakeLists.txt file). For instance, this PR enfores -O0 and certain compiler flags for the CCPP framework. We definitely do not want -O0 for production environments. There must be different options to set compilers for unit testing and for host models like the UFS, CAM/SIMA, NEPTUNE, and there must be a way for the host model to define compiler flags that overwrite any defaults (if we want to set defaults for cases other than running the tests).

There are two comments below that need fixing.

We also need to consider targeting the main branch for this PR due to the impact on the host modeling systems. This PR may have to come straight after the current develop was merged into main. And it must be written in a way that it allows ccpp-prebuild to continue to work, otherwise this PR (or the develop branch, if this PR gets merged into develop) will be blocked until all host models have moved to capgen. We don't want this, it was a heavy lift last time to get feature/capgen being merged into main after a long time of parallel development.

climbfuji · 2024-11-11T12:58:59Z

CMakeLists.txt

+  ADD_COMPILE_OPTIONS(-ggdb)
+  ADD_COMPILE_OPTIONS(-ffree-line-length-none)
+  ADD_COMPILE_OPTIONS(-cpp)
+elseif(${CMAKE_Fortran_COMPILER_ID} MATCHES "IntelLLVM")


ifort will be staying around for quite a while longer, we need this as an option, too.

Ah, okay, I'll add those back in and differentiate between ifort and ifx.

Added ifort support back but haven't set up a job to run it yet.

@climbfuji Would you be able to take a look at this and let me know if it's still not configured correctly and resolve the chain if so?

climbfuji · 2024-11-11T13:00:33Z

CMakeLists.txt

+option(BUILD_SHARED_LIBS "Build a static library" OFF)
+set(CCPP_VERBOSITY "0" CACHE STRING "Verbosity level of output (default: 0)")
+
+if(CCPP_FRAMEWORK_ENABLE_TESTS)


If CCPP_FRAMEWORK_ENABLE_TESTS is ON, then there needs to be a find_package call for pFUnit with the REQUIRED keyword. pfunit must come from the user-provided software stack. No implicit installs during cmake calls/build steps.

Understood, happy to make that change. I'm actually looking to make a container with all the dependencies/packages already installed so the environment better mimics our development environments to make the cmake call clean as well (we can pass in the pfunit path at configure time but it's clunky without modules).

Updated so the debug flags are only added for test runs. I would like to create a container with the dependencies and cut down on the turnaround time to run the tests but this will work for now until that gets added.

We should just move to a spack-stack container in the future

Would we be able to leverage that at NCAR or would that be site specific?

Anybody can. It's trivial to make a container that suits our needs, your needs, ...

climbfuji · 2024-11-11T13:11:49Z

test/capgen_test/test_reports.py


 Usage: python test_reports <build_dir> <database_filepath>
 -----------------------------------------------------------------------
 """


Is every test directory in test/ going to have a copy of this gigantic file?

I will be templating it so each test/test_reports.py file only sets the field variables and inherits the tests from a shared parent file. That's a detail that's on my list but wanted to get feedback on the present state before refactoring the other directories.

@climbfuji Would you be able to take another look at this now that the tests are refactored and let me know if this can be resolved?

mwaxmonsky · 2024-11-11T15:54:02Z

Hi @mwaxmonsky, thanks for tackling this.

So far, most of this looks like much needed cleanup!

There is one change I do not understand. The old (clumsy, wordy) run_test script tested both the Python and the bash interfaces to ccpp_datafile.py. Do the new tests test the bash interface? These are (or at least were) used by some build systems.

@gold2718 Yup! I made sure to be as faithful to the current coverage and made each of the shell tests their own unit tests at the bottom of the python file using the subprocess.run(...) API. If I missed anything on that front, I'm happy to add and address anything that might be missing.

mwaxmonsky · 2024-11-11T23:19:10Z

There is a lot of great stuff in this PR, thanks @mwaxmonsky.

My main concern with this PR is that it touches code/build files that aren't just used by the unit tests, but also by production code (e.g. the top-level CMakeLists.txt file). For instance, this PR enfores -O0 and certain compiler flags for the CCPP framework. We definitely do not want -O0 for production environments. There must be different options to set compilers for unit testing and for host models like the UFS, CAM/SIMA, NEPTUNE, and there must be a way for the host model to define compiler flags that overwrite any defaults (if we want to set defaults for cases other than running the tests).

There are two comments below that need fixing.

We also need to consider targeting the main branch for this PR due to the impact on the host modeling systems. This PR may have to come straight after the current develop was merged into main. And it must be written in a way that it allows ccpp-prebuild to continue to work, otherwise this PR (or the develop branch, if this PR gets merged into develop) will be blocked until all host models have moved to capgen. We don't want this, it was a heavy lift last time to get feature/capgen being merged into main after a long time of parallel development.

@climbfuji Happy to remove the -O0 flag and the other flags (I meant to make those target specific but wanted to get feed back on everything else before going further with the build), that was just in there from debugging and we definitely don't want that in there for production environments.

In terms of options from different compilers/flags, I'll look into different APIs for cmake and see what makes the most sense.

As for targeting main, I'm happy to pivot there but the tests for prebuild run successfully the same as on main. Is the issue that users would have to update their model code and their build integration with the framework? Just trying to understand the issue of targeting develop instead of main.

climbfuji · 2024-11-12T03:08:07Z

There is a lot of great stuff in this PR, thanks @mwaxmonsky.
My main concern with this PR is that it touches code/build files that aren't just used by the unit tests, but also by production code (e.g. the top-level CMakeLists.txt file). For instance, this PR enfores -O0 and certain compiler flags for the CCPP framework. We definitely do not want -O0 for production environments. There must be different options to set compilers for unit testing and for host models like the UFS, CAM/SIMA, NEPTUNE, and there must be a way for the host model to define compiler flags that overwrite any defaults (if we want to set defaults for cases other than running the tests).
There are two comments below that need fixing.
We also need to consider targeting the main branch for this PR due to the impact on the host modeling systems. This PR may have to come straight after the current develop was merged into main. And it must be written in a way that it allows ccpp-prebuild to continue to work, otherwise this PR (or the develop branch, if this PR gets merged into develop) will be blocked until all host models have moved to capgen. We don't want this, it was a heavy lift last time to get feature/capgen being merged into main after a long time of parallel development.

@climbfuji Happy to remove the -O0 flag and the other flags (I meant to make those target specific but wanted to get feed back on everything else before going further with the build), that was just in there from debugging and we definitely don't want that in there for production environments.

In terms of options from different compilers/flags, I'll look into different APIs for cmake and see what makes the most sense.

As for targeting main, I'm happy to pivot there but the tests for prebuild run successfully the same as on main. Is the issue that users would have to update their model code and their build integration with the framework? Just trying to understand the issue of targeting develop instead of main.

Yes, if you change the top-level CMakeLists.txt, you will impact the host models that use the ccpp-framework. UFS, SCM and NEPTUNE all use the ccpp-framework CMakeLists.txt.

mwaxmonsky · 2024-11-12T15:12:53Z

Also, this might be better saved for a discussion thread but I was wondering about the references to MPI and OpenMP. I don't see any openmp pragmas or types and currently the only reference to MPI is the mpi_comm in ccpp_types and even then we only declare a public reference but never use it internally.

I see a note in common.py about MPI:

# Maximum number of concurrent CCPP instances per MPI task
CCPP_NUM_INSTANCES = 200

and this value is used in the write function in mkstatic but it doesn't look like this is directly tied to the framework being build with MPI support.

If we don't need MPI or openmp at build time for the framework, would it make sense removing these references in the CMake and ccpp_types or are these actively being used and I'm just missing a key detail?

gold2718 · 2024-11-12T19:31:54Z

Currently, capgen generates omp_get_thread_num calls in the suite caps.
I don't think there is any MPI code generated currently by capgen.

climbfuji · 2024-11-12T19:50:46Z

ccpp-prebuild doesn't use any OpenMP calls in the auto-generated caps (everything comes from the host model via ccpp_t and its components, but the auto-generated caps and the code in src/ccpp_types.F90 need MPI. Removing the MPI dependency may break host models currently using ccpp_prebuild.

climbfuji · 2024-12-19T16:37:12Z

CMakeLists.txt

 #------------------------------------------------------------------------------
 # Set package definitions
 set(PACKAGE "ccpp-framework")
 set(AUTHORS "Dom Heinzeller" "Grant Firl" "Mike Kavulich" "Dustin Swales" "Courtney Peverley")


Should we add more people here? Those who regularly contribute code and are on the CCPP framework dev team (@mwaxmonsky for example???)

AUTHORS seems to be an environment variable that isn't used anywhere in the framework. Do we actually need this here?

If we do want to list the project authors it should probably be in documentation, maybe the top-level README.md?

I looked into this a while ago and I think pre-github days, this was one of the ways to list the authors when packaging a release through one of the CPack functions (or maybe there was an intent to use the variable at some point?) but now that we have a CODEOWNERS file, would it make sense to create a single source for the code authors?

There doesn't seem to be a standard way to manage contributors/authors if we want to list external contributions unfortunately so any solution we come up with will have to be hand built depending on how we want to integrate that capability as well.

If we don't need, then let's remove it.

Can we remove this line?

climbfuji

It would have been nice if we could have avoided the duplication of variable lists between Fortran and Python, but we can make this update another time. The changes here look great and we should merge this rather sooner than later.

climbfuji · 2025-07-11T16:47:52Z

CMakeLists.txt

 #------------------------------------------------------------------------------
 # Set package definitions
 set(PACKAGE "ccpp-framework")
 set(AUTHORS "Dom Heinzeller" "Grant Firl" "Mike Kavulich" "Dustin Swales" "Courtney Peverley")


Can we remove this line?

CMakeLists.txt

climbfuji · 2025-07-11T16:52:13Z

CMakeLists.txt

+  set (CMAKE_C_FLAGS "${CMAKE_C_FLAGS} ${OpenMP_C_FLAGS}")
+  set (CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} ${OpenMP_CXX_FLAGS}")


Can we do this with modern CMake syntax, i.e. only find the package here and then do

target_link_libraries(... OpenMP::OpenMP_C OpenMP::OpenMP_Fortran)

So I think I might be missing something. The _OPENMP pragmas are added to the capgen code but even forcing _OPENMP=1 when running these results in no issues. Is this a case of we need these flags for the ccpp_lib but without a host explicitly needing OpenMP or MPI, we won't see any issues when they are removed?

@mwaxmonsky I created mwaxmonsky#1 to better explain what I was thinking of.

Thanks @climbfuji! I pulled in those changes but 2 things come to mind:

If we are requiring OPENMP for the workflow, should we add another workflow without OpenMP?

I think the github runners support two cores, would it make sense to construct the tests to leverage two cores so we know that OpenMP is supported at runtime or is building/linking enough as a check?

IMO what we have is good enough for both 1 and 2 for now.

Okay, I'll leave everything as is. Thanks!

climbfuji · 2025-07-11T16:54:14Z

src/CMakeLists.txt

 add_library(ccpp_framework STATIC ${SOURCES_F90})
 target_link_libraries(ccpp_framework PUBLIC MPI::MPI_Fortran)
-set_target_properties(ccpp_framework PROPERTIES VERSION ${PROJECT_VERSION}
+set_target_properties(ccpp_framework PROPERTIES


See my comment above. Using the correct target_link_libraries for OpenMP will make line 18 unnecessary

test/utils/CMakeLists.txt

test/var_compatibility_test/test_var_compatibility_integration.F90

… output

This reverts commit 9e8498e.

Suggestions for PR NCAR#612

peverwhee

thanks @mwaxmonsky ! One small documentation addition request.

test/README.md

Co-authored-by: Courtney Peverley <[email protected]>

mkavulich · 2025-07-14T19:47:36Z

@mwaxmonsky I see you made a few more changes in response to the approving reviews. Are there any more changes you planned to make or is it ready to merge?

mwaxmonsky · 2025-07-14T19:49:13Z

@mwaxmonsky I see you made a few more changes in response to the approving reviews. Are there any more changes you planned to make or is it ready to merge?

@mkavulich I'm happy with where this is and don't have any additional changes in mind so unless there are any last minute comments, I think we're good to go!

…mpilers; run ccpp_prebuild.py tests in GitHub actions (#6) 1. Initial support for LLVM clang/flang; add guards to prevent using an unsupported compiler. 2. Enable basic CI testing for ccpp_prebuild.py and capgen.py on Atlantis with LLVM. Changes will be propagated to NCAR ccpp-framework main, but because of an impending update of the testing harness in the repository (NCAR#612) I will wait until after that PR is merged.

mwaxmonsky added 19 commits October 26, 2024 09:30

Re-adding pylint test.

bcf4097

Simplifying arg parser.

340c2f9

First pass refactor of tests into integrated stack.

1285ea9

Updating MPI cmake find with components.

c707b20

Adding mpi library.

fd66d0f

Updating mpi library.

278b2bd

Adding missing cmake.

9ce27bf

Actually building.

68f2575

Adding pytest dependency.

c256bed

Updating pytest binary name.

e7f84f4

Updating pytest install.

c886348

Updating pytest binary name.

20feac2

Updating test binary name in ctest to distinguish between other tests…

efff984

… with same binary name.

Minor CMake cleanup and temporarily removing MPI dependency.

84cdd96

Re-adding MPI for testing.

cc43fda

Resolving merge conflicts merging develop into testing-refactor branch.

890ea0e

Re-adding MPI linking and minor CMake cleanup.

cfe3921

Fixing CI job failure.

9b66008

Removing pylint workflow for different PR.

df44e3a

climbfuji requested changes Nov 11, 2024

View reviewed changes

climbfuji reviewed Nov 11, 2024

View reviewed changes

climbfuji reviewed Dec 19, 2024

View reviewed changes

mwaxmonsky added 8 commits July 7, 2025 09:28

Initial test README.md

b52536a

Removing unneeded override of CMAKE_MODULE_PATH.

ab128aa

Added documentation for building docs.

0cb54ce

Documentation update/cleanup.

5a4b60b

Add individual test run instructions.

996c108

Updating unit-test README

fd910da

Adding docs to run doctests.

8b33cb8

Minor formatting update.

674de31

mwaxmonsky requested review from climbfuji and peverwhee July 8, 2025 16:03

Merge branch 'develop' into testing-refactor

ebff27f

climbfuji approved these changes Jul 11, 2025

View reviewed changes

mwaxmonsky and others added 4 commits July 11, 2025 11:48

Adding missing newlines.

e10df98

Addressing formatting and BUILD_SHARED_LIBS comment from PR review.

6cfd499

Removing un-needed flags.

b8f5334

Suggestions to clean up OpenMP cmake integration and cmake diagnostic…

461f42e

… output

climbfuji mentioned this pull request Jul 11, 2025

Suggestions for PR https://github.com/NCAR/ccpp-framework/pull/612 mwaxmonsky/ccpp-framework#1

Merged

mwaxmonsky and others added 4 commits July 11, 2025 16:03

Adding ignore line for build directory.

9e8498e

Revert "Adding ignore line for build directory."

cda0028

This reverts commit 9e8498e.

Merge pull request #1 from climbfuji/feature/refactor_feedback_dom

3013128

Suggestions for PR NCAR#612

Adding missing newline.

da2d9d2

peverwhee approved these changes Jul 14, 2025

View reviewed changes

test/README.md Show resolved Hide resolved

mwaxmonsky and others added 3 commits July 14, 2025 10:58

Adding demo cmake command.

d2577a2

Minor comment cleanup.

75cbe98

Adding case specific documentation for regression tests.

ca34e38

Co-authored-by: Courtney Peverley <[email protected]>

mkavulich merged commit 74b2f22 into NCAR:develop Jul 14, 2025
19 checks passed

climbfuji mentioned this pull request Sep 24, 2025

Update capgen testing #471

Open

		set (CMAKE_C_FLAGS "${CMAKE_C_FLAGS} ${OpenMP_C_FLAGS}")
		set (CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} ${OpenMP_CXX_FLAGS}")

Testing refactor #612

Testing refactor #612

Uh oh!

Conversation

mwaxmonsky commented Nov 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mwaxmonsky commented Nov 11, 2024

Uh oh!

gold2718 commented Nov 11, 2024

Uh oh!

climbfuji left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mwaxmonsky commented Nov 11, 2024

Uh oh!

mwaxmonsky commented Nov 11, 2024

Uh oh!

climbfuji commented Nov 12, 2024

Uh oh!

mwaxmonsky commented Nov 12, 2024

Uh oh!

gold2718 commented Nov 12, 2024

Uh oh!

climbfuji commented Nov 12, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

climbfuji left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

mwaxmonsky commented Nov 11, 2024 •

edited

Loading