Tests: harden TestNetwork

[erlend-aasland]

• make tests deterministic
• use cleanup hooks

[codecov-commenter]

:warning: Please install the to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

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

Project coverage is 67.93%. Comparing base (c4560da) to head (0af94f7).

:exclamation: Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

[](https://app.codecov.io/gh/christiansandberg/canopen/pull/505?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Christian+Sandberg) ```diff @@ Coverage Diff @@ ## master #505 +/- ## ========================================== + Coverage 67.90% 67.93% +0.03% ========================================== Files 26 26 Lines 3116 3116 Branches 527 527 ========================================== + Hits 2116 2117 +1 + Misses 858 856 -2 - Partials 142 143 +1 ``` [see 1 file with indirect coverage changes](https://app.codecov.io/gh/christiansandberg/canopen/pull/505/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Christian+Sandberg)

[erlend-aasland]

I did some experiments with a full-blown CI matrix[^1] and discovered that test_send_periodic failed 99% of the times on the macOS CI, hence this PR. I think it would be valuable to run the CI on different platforms. See also #501 for a Windows specific (?) error.

[^1]: all supported Python versions x [windows, macos, linux]

[acolomb]

I think it would be valuable to run the CI on different platforms.

As long as GitHub covers the additional computation costs, sure why not. Although I think Python 3.8 (oldest supported) and the two most recent Python releases (3.11, 3.12 currently) should be sufficient.

[erlend-aasland]

I think it would be valuable to run the CI on different platforms.

As long as GitHub covers the additional computation costs, sure why not. Although I think Python 3.8 (oldest supported) and the two most recent Python releases (3.11, 3.12 currently) should be sufficient.

Sounds good. I'll pair it with a matrix of operating systems, post landing this PR.

[erlend-aasland]

I really appreciate the thoroughness of your reviews; thanks a lot!

[acolomb]

Oh, just noticed my commit message erroneously referred to heartbeats in the periodicity stuff. Though this is lower level, I just got confused.

Again, thank YOU for putting in all this work in the first place. Otherwise, there would be less to review but also no improvements.

[erlend-aasland]

Oh, just noticed my commit message erroneously referred to heartbeats in the periodicity stuff. Though this is lower level, I just got confused.

No sweat, you're fine; as you said yourself, no Git log is perfect :)

[erlend-aasland]

Hah, eating my words already today! This is not deterministic enough, and it will break on macOS CI 😆 So much for my "pretty certain" https://github.com/christiansandberg/canopen/pull/505#discussion_r1670232695.

So, I learned that .update() very often messes up the timing! The test needs to take this into account, so the resulting code will be a little bit more verbose. I'll create a follow-up PR.

BTW, I still can't get reproducible tests using .recv(). Perhaps it is buggy?

[acolomb]

Well, apparently only the socketcan interface in python-can implements the required modify_data() method. Otherwise, it falls back to stopping and restarting the task, which the comment in update() explains as messing up the period.

BTW, I still can't get reproducible tests using .recv(). Perhaps it is buggy?

Haven't really looked at the implementation, but I thought it was just a lower level call in the underlying python-can library. It's probably a difference between waiting / polling messages and installing a notifier callback (which network.subscribe() does.

[erlend-aasland]

It's probably a difference between waiting / polling messages and installing a notifier callback (which network.subscribe() does.

Yep, sounds plausible.