Updates to unit tests in test_metadata_scheme_file.py:

[JulieSchramm]

• Update unit tests to include ccpp-table-properites
• Rename sample files preproc_defs_test*.meta to be more descriptive
• Rename sample files preproc_defs_test*.F90 to be more descriptive

pylint rated at 10.00/10 for test_metadata_scheme_file.py
All tests pass for test_metadata_scheme_file.py

Still need to update test_metadata_table.py

[climbfuji]

Looked at it briefly, nothing that sticks out.

[gold2718]

This looks good, however, if we are testing the new metadata format, please add some variations in the dependencies and relative_path table properties and test that the correct values are recovered. Also test headers without one or both of these properties (they are optional).

[JulieSchramm]

Can you point me to a case where this relative_path is actually used?

…

On Wed, Sep 16, 2020 at 5:03 PM goldy ***@***.***> wrote: ***@***.**** requested changes on this pull request. This looks good, however, if we are testing the new metadata format, please add some variations in the dependencies and relative_path table properties and test that the correct values are recovered. Also test headers without one or both of these properties (they are optional). ------------------------------ In test/unit_tests/sample_scheme_files/CCPPeq1_var_in_fort_meta.meta <#323 (comment)>: > @@ -1,5 +1,11 @@ +[ccpp-table-properties] + name = CCPPeq1_var_in_fort_meta + type = scheme + dependencies = Is this purposely testing a blank dependencies line? This feature should be checked, i.e., a blank or absent line should be tested as None (via table.dependencies) and a value should be tested to see if it is the correct string. Also, the new relative_path keyword should be tested in the same manner. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#323 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AA3WNU2GFJNYMCQAORCNUZDSGE74XANCNFSM4RPDUDFA> .

[JulieSchramm]

Also, shouldn't testing of the contents of the table be done with the test_metadata_table.py unit tests, and not the test_metadata_scheme_file.py unit tests?

…

On Thu, Sep 17, 2020 at 8:26 AM Julie Schramm ***@***.***> wrote: Can you point me to a case where this relative_path is actually used? On Wed, Sep 16, 2020 at 5:03 PM goldy ***@***.***> wrote: > ***@***.**** requested changes on this pull request. > > This looks good, however, if we are testing the new metadata format, > please add some variations in the dependencies and relative_path table > properties and test that the correct values are recovered. Also test > headers without one or both of these properties (they are optional). > ------------------------------ > > In test/unit_tests/sample_scheme_files/CCPPeq1_var_in_fort_meta.meta > <#323 (comment)>: > > > @@ -1,5 +1,11 @@ > +[ccpp-table-properties] > + name = CCPPeq1_var_in_fort_meta > + type = scheme > + dependencies = > > Is this purposely testing a blank dependencies line? This feature should > be checked, i.e., a blank or absent line should be tested as None (via > table.dependencies) and a value should be tested to see if it is the > correct string. > Also, the new relative_path keyword should be tested in the same manner. > > — > You are receiving this because you authored the thread. > Reply to this email directly, view it on GitHub > <#323 (review)>, > or unsubscribe > <https://github.com/notifications/unsubscribe-auth/AA3WNU2GFJNYMCQAORCNUZDSGE74XANCNFSM4RPDUDFA> > . >

[gold2718]

Can you point me to a case where this relative_path is actually used?

No, it is just a string or None. It is used by the build system which is outside CCPP Framework testing.

[gold2718]

Also, shouldn't testing of the contents of the table be done with the
test_metadata_table.py unit tests, and not the test_metadata_scheme_file.py
unit tests?

Yes, I agree. Because of that, you can just omit the dependencies keyword from the scheme file test metadata files.

[climbfuji]

@gold2718 @JulieSchramm following up on this PR - is there anything that needs to be changed before we can get @gold2718's approval and merge it?

[climbfuji]

I did approve this PR in its current form previously, doing it again.

[gold2718]

Looks okay now, thanks.