ccpp-framework icon indicating copy to clipboard operation
ccpp-framework copied to clipboard

Added test using a DDT host object to pass information

Open gold2718 opened this issue 1 year ago • 2 comments

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 avatar Sep 23 '24 16:09 gold2718

A couple of thoughts:

  • Instead of a new Fortran test, I could move the ccpp object into a different test (e.g., the advection_test).
  • Another test mod I thought of but did not try is to move the error variables (and/or the loop variables) out of the host-model call strings and into module data. Is anyone interested in this feature?

gold2718 avatar Sep 23 '24 16:09 gold2718

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.

gold2718 avatar Sep 24 '24 17:09 gold2718

@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!

climbfuji avatar Oct 16 '24 13:10 climbfuji

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.

mwaxmonsky avatar Oct 18 '24 13:10 mwaxmonsky

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

mkavulich avatar Nov 21 '24 16:11 mkavulich

Any more takes for this PR? It's got two approvals so far, from NRL and CGD. Can we get one from NOAA, please?

climbfuji avatar Dec 09 '24 16:12 climbfuji

@dustinswales or @grantfirl Please review when it's convenient for you so that we have the green light from NOAA to merge. Thanks!

climbfuji avatar Dec 09 '24 22:12 climbfuji