Skip to content

Conversation

@andrewleap-optimizely
Copy link
Contributor

Summary

  • Fix forced decision test with thread safe call counter
  • Fix polling config manager tests from interfering with their own results
  • Add wrapper class to allow actual testing of the polling thread

Due to a difference in the way pypy implements threading, tests utilizing threading have been intermittently failing the pypy CI. For the polling config manager, depending on when the polling thread started, it would sometimes poison the results, due to the mock library not interacting with the thread correctly.
For the forced decision test, mock's call_count is not thread safe, so the calls were sometimes not counted correctly.

Additionally, some of the polling config manager tests were calling fetch_datafile manually and then confirming that the polling thread was still alive. Since fetch_datafile wasn't being run by the thread, this check was not actually checking what it was purported to check.

Issues

  • Should address these pypy errors:
    FAILED tests/test_config_manager.py::PollingConfigManagerTest::test_fetch_datafile__status_exception_raised
    AssertionError: 'New Time' !=
    FAILED tests/test_user_context.py::UserContextTest::test_forced_decision_sync_return_correct_number_of_calls
    AssertionError: 200 != 137

@andrewleap-optimizely andrewleap-optimizely requested a review from a team as a code owner May 18, 2022 18:30
@andrewleap-optimizely andrewleap-optimizely changed the title Fix tests that utilize threading from failing with pypy fix: tests that utilize threading from failing with pypy May 19, 2022
@Mat001 Mat001 requested a review from jaeopt May 19, 2022 16:20
Copy link
Contributor

@Mat001 Mat001 left a comment

Choose a reason for hiding this comment

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

I think it looks ok to me.
I'll re-run pypy tests on gitactions multiple times to see if they are stable.

Copy link
Contributor

@jaeopt jaeopt left a comment

Choose a reason for hiding this comment

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

LGTM - comments about the test changes will be helpful

@msohailhussain
Copy link
Contributor

msohailhussain commented May 31, 2022

@andrewleap-optimizely Can you please add expectations of threading.Thread.start and requests.get that these expects the methods are triggered.

Copy link
Contributor

@msohailhussain msohailhussain left a comment

Choose a reason for hiding this comment

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

lgtm. please address comments before merge.

@andrewleap-optimizely andrewleap-optimizely merged commit eee3aa0 into master Jun 1, 2022
@andrewleap-optimizely andrewleap-optimizely deleted the aleap/fix_threading_tests branch June 1, 2022 19:39
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