Skip to content

Conversation

@erlend-aasland
Copy link
Contributor

No description provided.

@erlend-aasland
Copy link
Contributor Author

I'm not satisfied with test_emcy_consumer_wait; I'll try to come up with something less smelly tomorrow.

@codecov-commenter
Copy link

codecov-commenter commented Aug 11, 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 70.94%. Comparing base (28ee18c) to head (e7ca8a1).

❗ 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     #529      +/-   ##
==========================================
+ Coverage   70.36%   70.94%   +0.57%     
==========================================
  Files          26       26              
  Lines        3111     3111              
  Branches      526      526              
==========================================
+ Hits         2189     2207      +18     
+ Misses        794      776      -18     
  Partials      128      128              

see 1 file with indirect coverage changes

@erlend-aasland
Copy link
Contributor Author

I'm not satisfied with test_emcy_consumer_wait; I'll try to come up with something less smelly tomorrow.

Oh well, perhaps it's fine; testing "wait functions" is always a hassle.

Moreover, perhaps test_emcy_get_desc got too detailed; let me know, and I'll gladly loosen it up.

Copy link
Member

@acolomb acolomb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me and the tests actually pass. If you're satisfied, we can merge it right away.

@erlend-aasland
Copy link
Contributor Author

Looks good to me and the tests actually pass. If you're satisfied, we can merge it right away.

If you're happy, I'm happy! Feel free to land it.

@acolomb acolomb merged commit 5004363 into canopen-python:master Aug 12, 2024
@erlend-aasland erlend-aasland deleted the test/emcy branch August 12, 2024 07:28
@acolomb
Copy link
Member

acolomb commented Aug 12, 2024

@erlend-aasland
Copy link
Contributor Author

@erlend-aasland
Copy link
Contributor Author

Ah, it was the smelly test_emcy_consumer_wait!

@acolomb
Copy link
Member

acolomb commented Aug 12, 2024

Maybe joining the timer thread in between the different steps would help? I don't really see how it fails yet, except for maybe 25 ms being too short to work accurately and reproducibly in the CI environment.

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