Skip to content

Tests: harden TestNetwork #505

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Jul 9, 2024

Conversation

erlend-aasland
Copy link
Contributor

  • make tests deterministic
  • use cleanup hooks

- make tests deterministic
- use cleanup hooks
@codecov-commenter
Copy link

codecov-commenter commented Jul 8, 2024

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

All modified and coverable lines are covered by tests ✅

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

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

Additional details and impacted files

Impacted file tree graph

@@            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

@erlend-aasland
Copy link
Contributor Author

erlend-aasland commented Jul 8, 2024

I did some experiments with a full-blown CI matrix1 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.

Footnotes

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

@acolomb
Copy link
Member

acolomb commented Jul 9, 2024

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 erlend-aasland marked this pull request as draft July 9, 2024 08:05
- check the number of triggers
- roughly check the interval
@erlend-aasland erlend-aasland marked this pull request as ready for review July 9, 2024 08:48
@erlend-aasland
Copy link
Contributor Author

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.

@acolomb acolomb merged commit 3aa509d into canopen-python:master Jul 9, 2024
3 checks passed
@erlend-aasland
Copy link
Contributor Author

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

@erlend-aasland erlend-aasland deleted the test/send-periodic branch July 9, 2024 11:07
@acolomb
Copy link
Member

acolomb commented Jul 9, 2024

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
Copy link
Contributor Author

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
Copy link
Contributor Author

Hah, eating my words already today! This is not deterministic enough, and it will break on macOS CI 😆 So much for my "pretty certain" #505 (comment).

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
Copy link
Member

acolomb commented Jul 9, 2024

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
Copy link
Contributor Author

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

Yep, sounds plausible.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants