Add unit tests for SDF reading#5
Conversation
Fix xmllint checking (some xmllint versions do not return error codes) Update regex usage for fortran_conditional
climbfuji
left a comment
climbfuji
left a comment
There was a problem hiding this comment.
I am happy with these additions, thanks for your efforts. They will increase the size of my PR to develop by quite a bit, but I don't think that's a problem - it's almost entirely due to new files.
| for fpath in glob.iglob(os.path.join(_TMP_DIR, '*.*')): | ||
| if os.path.exists(fpath): | ||
| os.remove(fpath) | ||
| # End if |
There was a problem hiding this comment.
I noticed that sometimes these ends start with an uppercase E, sometimes with a lowercase e. I don't necessarily mind at this point, but if you do then maybe you want to make them consistent?
There was a problem hiding this comment.
A long time ago, our agreement was that I could keep these visual placeholders (really great for those of us with limited eyesight) but they should all be lowercase (I think they were all capitalized at that point). I have no idea how some of these are still around. Do you want me to make a clean up run through the scripts?
There was a problem hiding this comment.
I think it's ok to leave them as is. I was hoping to revisit the question whether there is still a need for these placeholders at our "big" meeting on Dec 4 (with all options on the table). We can make a simple sed sweep after that meeting if we keep the placeholders.
There was a problem hiding this comment.
More important is that we get your branch rebased onto develop after my PR (nested suites) was approved and merged, and that we fix the regex parsing error that forced me to revert your changes on top of my branch.
I am traveling back from SC25 today and won't get much work done. I will be on PTO the entire next week, too. Therefore, apologies for any delays from my end over the next 1-2 weeks.
… validation fails (#708) Handle and test xmllint versions that return a zero exit code even if validation fails 1. In `scripts/parse_tools/xml_tools.py`, handle `xmllint` versions that return a zero exit code even if the validation fails. Simply logic, remove unused code, and update `test/unit_tests/test_sdf.py` accordingly 2. Create `test/unit_tests/xmllint_wrapper/xmllint` wrapper that emulates the behavior of "bad" `xmllint` which returns a zero exit code even if the validation fails, and test in GitHub actions Notes. 1. This PR is based on code suggested by @gold2718 in a PR that got merged but later reverted due to problems with (unrelated) regular expression changes - see climbfuji#5. Credits to @gold2718 for this contribution. 2. `call_command` was only used in one place (validate XML files) and contained overly complicated logic to deal with optional arguments that were unnecessary. With the need to parse stdout/stderr from the subprocess in the `validate_xml` function, there would have been even more code duplication and unnecessary logic. 3. When #691 was merged, `xmllint` became a hard requirement, therefore we can remove logic that allows silent failures of the XML validation step when `xmllint` cannot be found (silent failures in general are a bad idea imo). User interface changes?: No Fixes #700 Testing: unit tests: `pytest` is now run in GitHub actions with both "good" and "bad" xmllint versions manual testing: ran Python `doctest` on the updated `xml_tools.py` file, ran `pytest` locally on my laptop with "good" and "bad" xmllint
2 participants
Fix xmllint checking (some xmllint versions do not return error codes)
Update regex usage for fortran_conditional
I think these tests should pass