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

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

Merged
merged 7 commits into from
Dec 11, 2024

Conversation

gold2718
Copy link
Collaborator

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

Fix problems so that test passes
Improve formatting for readability
@gold2718 gold2718 added the bugfix Fix for issue with 'bug' label. label Sep 23, 2024
@gold2718 gold2718 added this to the capgen unification milestone Sep 23, 2024
@gold2718 gold2718 self-assigned this Sep 23, 2024
@gold2718
Copy link
Collaborator Author

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?

Copy link
Collaborator

@climbfuji climbfuji left a comment

Choose a reason for hiding this comment

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

Do we really need such a complex test for just making sure that errmsg and errflg can be part of a DDT? At first glance, it seems to me that we can do away with all the temp_* stuff in this ddthost_test.

scripts/ccpp_suite.py Show resolved Hide resolved
test/ddthost_test/environ_conditions.F90 Show resolved Hide resolved
test/ddthost_test/make_ddt.F90 Outdated Show resolved Hide resolved
@gold2718
Copy link
Collaborator Author

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
Copy link
Collaborator

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

Copy link
Collaborator

@mwaxmonsky mwaxmonsky 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 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.

test/ddthost_test/test_reports.py Outdated Show resolved Hide resolved
scripts/metavar.py Outdated Show resolved Hide resolved
test/ddthost_test/make_ddt.F90 Show resolved Hide resolved
test/ddthost_test/temp_adjust.F90 Show resolved Hide resolved
test/ddthost_test/temp_calc_adjust.F90 Show resolved Hide resolved
test/ddthost_test/CMakeLists.txt Outdated Show resolved Hide resolved
test/ddthost_test/environ_conditions.F90 Outdated Show resolved Hide resolved
test/ddthost_test/test_reports.py Outdated Show resolved Hide resolved
test/ddthost_test/test_reports.py Outdated Show resolved Hide resolved
test/ddthost_test/test_reports.py Outdated Show resolved Hide resolved
@mwaxmonsky
Copy link
Collaborator

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.

Copy link
Collaborator

@climbfuji climbfuji 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 accommodating my change requests

Copy link
Collaborator

@mwaxmonsky mwaxmonsky left a comment

Choose a reason for hiding this comment

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

Thanks @gold2718! One minor bug fix that I think got missed but otherwise looks good!

test/ddthost_test/CMakeLists.txt Outdated Show resolved Hide resolved
@mkavulich
Copy link
Collaborator

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

Modify run_test so that verbose is set to level 2 by default.
Tested with verbose level 0, 1, and 2.
@climbfuji
Copy link
Collaborator

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
Copy link
Collaborator

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

Copy link
Collaborator

@dustinswales dustinswales left a comment

Choose a reason for hiding this comment

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

Sorry for the delay, this got lost in my holiday vacation shuffle. Looks good to me.

@climbfuji climbfuji merged commit 48443af into NCAR:develop Dec 11, 2024
19 checks passed
@climbfuji climbfuji deleted the development branch December 11, 2024 19:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugfix Fix for issue with 'bug' label.
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

5 participants