-
Notifications
You must be signed in to change notification settings - Fork 64
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 options for pre-registered DDT names #644
base: develop
Are you sure you want to change the base?
Conversation
Add ccpp_constituent_prop_ptr_t as default DDT Add test for 'unknown' DDT variables
I wasn't sure how to best add a test for a one-off script such as ccpp_fortran_to_metadata.py so I just plunked a test script and example file in the test directory. Perhaps @mwaxmonsky can provide guidance that is more or less inline with his testing refactoring work . |
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.
I'll defer to @mwaxmonsky for the best way to test this, otherwise this looks good to me
Thanks for adding this in @gold2718! In terms of setup, this feels in the spirit of the tests in the If I can get it working, would there be any concerns with using something like: import unittest
import subprocess
import tempfile
import difflib
def test_pre_registered_ddt_type(unittest.TestCase):
script = "..." #will follow current shell implementation.
d = tempfile.TemporaryDirectory(dir=...) #Will be the same temp directory from script.
subprocess.run(script)
expected_data = fopen(...)
actual_file = fopen(...)
deltas = list(difflib.ndiff(expected_data.readlines(), actual_file.readlines()))
assertEqual([], deltas)
d.cleanup() Again, I'm still flushing out a working version and if there is a benefit to using the script approach, I'm happy to add that capability to the python test harness if I can so the script isn't needed. |
@mwaxmonsky, I have no problem with your approach. I slapped the bash script together because it is easy (no subprocess stuff to set up and analyze). To make this work, I recognize a consistent approach is best. Do you want me to use (and fix) your script as a substitute for test_fortran_to_metadata.sh? |
I would say go with this as is and I can scoop it up into the refactor (or the next one at the current pace) since the test setup isn't a priority and I'm having to work through some other scenarios that will be useful in other places. The only thing I would suggest is adding this to one of the workflow files as I don't think it is currently run and will be missed in the CI jobs. |
@gold2718 Thanks for adding this feature. For the SCM I did something much less elegant for the mpi_Comm type |
Thanks, done! |
Add options for pre-registered DDT names
Add an option for pre-registered DDT names
Add
ccpp_constituent_prop_ptr_t
as a default (pre-registered) DDTAdd test for 'unknown' DDT variables
User interface changes?: Yes
Added a new option,
--ddt-names
, to allow users to simply add DDT names that allow generation of a CCPP metadata template from a Fortran source. Because it is an option, it is backwards compatible.Fixes: #643
ccpp_fortran_to_metadata.py errors out when given a file that has a constituent properties object
Testing:
test removed: None
unit tests: NA
system tests: NA
manual testing: Added a manual test that tests this bug fix