XLSForm / pyxform

A Python package to create XForms for ODK Collect.
BSD 2-Clause "Simplified" License
80 stars 136 forks source link

Improve xml hierarchy test methods, refactor test_trigger.py tests to use them #570

Closed lindsay-stevens closed 2 years ago

lindsay-stevens commented 2 years ago

Closes #563

Why is this the best possible solution? Were any other approaches considered?

This follows on from #548 to add the discussed xml__xpath_match kwarg for PyxformTestCase and to switch to the lxml package from ElementTree for the purposes of test XPath evaluation. The benefit of lxml is that it's XPath 1.0 processor can do a lot more than ElementTree's XPath subset. Gains include being able to write compound predicates (with and) in one predicate instead of like .//bind[1=1][1=1], the use of axes, and numerous XPath functions. Also it allows the old code for switching XML tokenizers for Python 3.7 to be removed. More thorough tests for the xml__xpath* matchers are added as well.

The way around the default namespace limitation in lxml (mentioned in #548) was to tell the XPath processor about the same namespace URI/URL but assign a non-empty and not-None prefix to it. The lxml docs say the namespace match is actually on the URI not the prefix, so it's a bit like SQL aliases in different SELECT clauses. Since the default namespace is http://www.w3.org/2002/xforms I went with a prefix of x for this purpose. The assignment happens in pyxform_test_case.py on L231 which is finally used at L556. This means that in any usages of the xml__xpath* test matchers, XPath expressions referring to elements in the default namespace need to be written like .//x:bind instead of .//bind. Attributes without a prefix don't require this x prefix. There are numerous examples in test_pyxform_test_case.py.

This PR also delivers on the main purpose of #563 to refactor the test_trigger.py tests to use the new xml__xpath* matchers. With above improvements and addition of xml__xpath_match, it was all possible with xml__xpath_match.

Further details in the commit messages.

What are the regression risks?

This is a new testing feature so there would be no regressions unless other branches had already started using it.

It's possible that the refactors of test_trigger.py test are not perfect, despite being careful. Each of these tests passes. The XPath results can be checked by setting debug=True, which prints out the XML document, each XPath expression and what it matched.

Does this change require updates to documentation? If so, please file an issue here and include the link below.

Not applicable.

Before submitting this PR, please make sure you have:

codecov-commenter commented 2 years ago

Codecov Report

Merging #570 (161c3e9) into master (6ca3484) will not change coverage. The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #570   +/-   ##
=======================================
  Coverage   84.20%   84.20%           
=======================================
  Files          25       25           
  Lines        3672     3672           
  Branches      865      865           
=======================================
  Hits         3092     3092           
  Misses        440      440           
  Partials      140      140           

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 6ca3484...161c3e9. Read the comment docs.

lindsay-stevens commented 2 years ago

Thanks @lognaturel I agree and have added a section to the README for this. I've focussed on xml__xpath_match because after having used it I think that will probably be all that's needed in >95% of cases.

lindsay-stevens commented 2 years ago

Appveyor build error seems to be random pip error, I only added to the Readme since the passing build in baec985