Closed carlescufi closed 2 years ago
@weiwei FYI
I think it would be worth adding a unit test that fails with lxml. This reduces the risk that someone optimizes this fix away as it seems redundant.
I think it would be worth adding a unit test that fails with lxml. This reduces the risk that someone optimizes this fix away as it seems redundant.
Thanks for the feedback! Not sure I get it. This never failed with lxml
, only with xml
. With this patch it doesn't fail with either. So not sure what the unit test would check?
Well, then the unit test should check that it does not fail (well work correctly) for either, so that the test fails if someone undoes the fix.
Well, then the unit test should check that it does not fail (well work correctly) for either, so that the test fails if someone undoes the fix.
OK, I'll try to come up with a unit test for this.
@EnricoMi I added a unit test for this, as well as fixing an existing one. I verified that it fails when lxml
is not installed and the first commit in this PR is not applied. Together with #103, this should ensure that we test this with both XML implementations.
Base: 98.36% // Head: 98.38% // Increases project coverage by +0.02%
:tada:
Coverage data is based on head (
8124c97
) compared to base (af4f061
). Patch coverage: 100.00% of modified lines in pull request are covered.
:umbrella: View full report at Codecov.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.
@carlescufi Thanks for the PR!
@carlescufi Thanks for the PR!
No problem! Thanks for merging!
The iteration in TestCase.result.setter did not work properly when using xml.etree instead of lxml.etree. This was because the Element.remove() method was being invoked while iterating the elements, and apparently xml.etree does not support that. Instead, use a prepopulated lits of Elements to iterate and remove.
Fixes #99.
Signed-off-by: Carles Cufi carles.cufi@nordicsemi.no