Fix sros2 tests on Windows Debug.

clalancette commented 6 months ago

From what I can tell, it looks like lxml has a bug where it doesn't properly track references to objects via the find() method. This manifests on Windows debug as a crash after we have stopped using the object, but I believe that by that point the underlying memory has already been freed. Windows Debug in particular is sensitive to this.

Fix it by doing a deepcopy of the object returned from the find(). This code isn't performance sensitive, so it shouldn't be a big deal to do it here, and it fixes the bug in my testing.

mikaelarguedas commented 6 months ago

Thanks 🙏

Ideally I'd like to keep separate the behavioral fix and API/signature change to streamline fix back porting to older distros. Regarding the mutable defaults 👍 (hence the frecebtly added file), however what would you think about making all ARGS keyed-args and keeping the order as is ?

Finally is it possible to trigger a windows debug job once the PR splitted to make sure the CI is happy about it ?

codecov[bot] commented 6 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 88.94%. Comparing base (fb22661) to head (4f136df).

Additional details and impacted files

```diff @@ Coverage Diff @@ ## rolling #317 +/- ## =========================================== + Coverage 88.92% 88.94% +0.01% =========================================== Files 24 24 Lines 614 615 +1 Branches 64 64 =========================================== + Hits 546 547 +1 Misses 50 50 Partials 18 18 ``` | [Flag](https://app.codecov.io/gh/ros2/sros2/pull/317/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=ros2) | Coverage Δ | | |---|---|---| | [unittests](https://app.codecov.io/gh/ros2/sros2/pull/317/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=ros2) | `88.94% <100.00%> (+0.01%)` | :arrow_up: | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=ros2#carryforward-flags-in-the-pull-request-comment) to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

clalancette commented 6 months ago

however what would you think about making all ARGS keyed-args and keeping the order as is ?

Actually, to be perfectly frank we could just leave the order as-is and remove all of the default arguments. This function is always called with all of its arguments. So I think I'll open a separate PR to do that.

clalancette commented 6 months ago

Here is just CI on Windows Debug. Assuming this is successful, I'll run full CI on it: