NCAR / ccpp-framework

Common Community Physics Package (CCPP)
http://www.dtcenter.org/community-code/common-community-physics-package-ccpp/
Other
26 stars 64 forks source link

Added test using a DDT host object to pass information #591

Open gold2718 opened 1 month ago

gold2718 commented 1 month ago

Added test using a DDT host object to pass information Fix problems so that test passes Improve formatting for readability

User interface changes?: No

Fixes: #589

Testing: test removed: None unit tests: Pass system tests: Pass, added DDT host object test manual testing: Ran doctests, examined generated code for system tests

gold2718 commented 1 month ago

A couple of thoughts:

gold2718 commented 1 month ago

Do we really need such a complex test for just making sure that errmsg and errflg can be part of a DDT?

That's what I thought which is why I made this comment.

climbfuji commented 1 month ago

@gold2718 As discussed on Monday, we should proceed with this PR, including adding the new test, once the comment on what base_only means (minimal documentation in the code?) is addressed and the other typo is fixed. Thanks!

mwaxmonsky commented 1 month ago

Thanks for getting this in @gold2718! Just had a few recommendations for style and consistency (and 1 or 2 functional changes) but just let me know if any of those are problematic, otherwise nothing major.

I did notice the test logs in the runner are printing out the error message:

-- Running: /home/runner/work/ccpp-framework/ccpp-framework/scripts/ccpp_capgen.py --host-files test_host_data.meta,test_host_mod.meta,test_host.meta --scheme-files cld_suite_files.txt --suites cld_suite.xml --host-name test_host --output-root /home/runner/work/ccpp-framework/ccpp-framework/test/at_build/ccpp --debug
-- xmllint not found, could not validate file /home/runner/work/ccpp-framework/ccpp-framework/test/advection_test/cld_suite.xml

If these tests should be failing if xmllint isn't on the path, I'm torn as to how to handle that as this looks like a pre-existing issue but it should be as simple as adding libxml2-utils to the capgen yaml workflow file on the apt-get install ... line but I would have to do some more testing on that front.

If this is outside the scope of this PR, we can make an issue to address that in a different PR.

Regarding the xmllint dependency error, let's hold off on any changes with regard to it here and we will address it if need be in #601.

mkavulich commented 20 hours ago

@dustinswales Can you take a look at this to make sure it's fine by you?