Skip to content

Possible fix for tests sometimes failing #828

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 2 commits into from
May 8, 2020

Conversation

sou1hacker
Copy link
Contributor

@sou1hacker sou1hacker commented May 6, 2020

Fixed a minor issue in tests where I think previously we were sometimes getting some left over messages from the first cyclic thread, causing the tests to fail.

One example PR where the tests were failing due to this: #825
Travis link: https://travis-ci.org/github/hardbyte/python-can/jobs/683606723

@codecov
Copy link

codecov bot commented May 6, 2020

Codecov Report

Merging #828 into develop will decrease coverage by 2.66%.
The diff coverage is n/a.

@@             Coverage Diff             @@
##           develop     #828      +/-   ##
===========================================
- Coverage    69.83%   67.16%   -2.67%     
===========================================
  Files           70       70              
  Lines         6603     6603              
===========================================
- Hits          4611     4435     -176     
- Misses        1992     2168     +176     

@sou1hacker sou1hacker marked this pull request as ready for review May 6, 2020 15:27
@pierreluctg pierreluctg self-requested a review May 6, 2020 15:33
@sou1hacker
Copy link
Contributor Author

Original PR: #781 (@tysonite )

@tysonite
Copy link
Contributor

tysonite commented May 6, 2020

Ohh, I see the issue, thanks for fixing it!

@hardbyte hardbyte merged commit ffbdab6 into hardbyte:develop May 8, 2020
@karlding
Copy link
Collaborator

karlding commented May 9, 2020

Maybe this obvious and I'm missing something, but I don't see how this solves the problem if you're receiving messages due to the thread loop condition check racing against the flag being set when stop is called...

Calling reset_mock resets all the call attributes on the mock, which imo is correct. And constructing a new mock doesn't provide a synchronization point or barrier, which is what's needed in the first place.

I would have expected the solution to either create a barrier before starting the next set of assertions, or prevent the race from affecting the test by explicitly synchronizing on the last message being delivered. You can't really create a barrier since the send thread isn't controlled by the test (stop would have to do so, but those aren't the semantics it provides), so your options are either:

  1. Teardown the bus and start a new one on a separate channel
  2. Wait for >= 1 period so the messages get flushed before continuing the test

@sou1hacker
Copy link
Contributor Author

The first approach I committed in this PR actually had a sleep between testing the first scenario and the second. However, this approach is slightly better since we don't waste time on sleeping and I don't think we are compromising on the existing test itself in any way.

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

Successfully merging this pull request may close these issues.

6 participants