Make is_active for projector return quickly #326
Conversation
This change makes the projector plugin start a separate thread to compute whether it is active. This allows the `is_active` method to return immediately. Otherwise, it may take a long time to return, which could cause issues - see #301 We can actually compute the _is_active property in less time by halting the other thread when we know we have 1 entry in the config object (instead of computing all the values of the object), but computing the config object should usually not take severely long anyway so the added complexity and duplicated logic does not seem worth it.
| def _get_thread_for_determining_is_active(self): | ||
| """Gets the currently running thread for determining is_active. | ||
|
|
||
| This method is useful for testing. |
There was a problem hiding this comment.
Why? This method is only used in tests, and all of its uses can be replaced with the direct field access. (They're both private, so it's not like using a method is more principled!)
| tf.test.mock.patch('threading.Thread', FakeThread).start() | ||
|
|
||
| # The projector plugin has not yet determined whether it is active, but it | ||
| # should have started a thread to determine that. |
There was a problem hiding this comment.
"Should have started" should read "should now start".
| # Embedding data is available. | ||
| # The is_active method makes use of a separate thread, so we mock threading | ||
| # behavior to make this test deterministic. | ||
| tf.test.mock.patch('threading.Thread', FakeThread).start() |
There was a problem hiding this comment.
This whole stubbing business is far more complicated than it needs to be. Why not just:
patcher = tf.test.mock.patch('threading.Thread.start', autospec=True)
mock = patcher.start()
self.addCleanup(patcher.stop) # remove `tearDown`
mock.assert_not_called()
self.assertFalse(self.plugin.is_active())
mock.assert_called_once_with(self.plugin._thread_for_determining_is_active)
self.assertFalse(self.plugin.is_active())
mock.assert_called_once_with(self.plugin._thread_for_determining_is_active)
self.plugin._thread_for_determining_is_active.run()
self.assertTrue(self.plugin.is_active())There was a problem hiding this comment.
I think the one hurdle here is that the mock doesn't actually run the logic of the thread, so self.assertTrue(self.plugin.is_active()) fails. Do you know of a way around that?
There was a problem hiding this comment.
Yes: call thread.run(), as in the snippet above. This will invoke the thread's handler synchronously.
There was a problem hiding this comment.
That works and removes the complexity of FakeThread. Wonderful idea.
1 alteration I made was replace mock.assert_not_called() with self.assertEqual(0, mock.call_count) because TF's version of mock lacked assert_not_called.
| @@ -250,7 +261,49 @@ def is_active(self): | |||
| Returns: | |||
| A boolean. Whether this plugin is active. | |||
There was a problem hiding this comment.
It's clear that is_active returns whether the plugin is active. This docstring should indicate what that means. ("The projector plugin is active when…")
There was a problem hiding this comment.
There are two aspects of the logic that aren't tested and would be fairly easy to test: (1) if the plugin is found to be active, further calls to is_active do not start a new thread; (2) if the plugin is found to be inactive, further calls to is_active do spawn a new thread. I'd recommend including these tests, but it's up to you, so I'm approving to unblock you.
| mock = patcher.start() | ||
| self.addCleanup(patcher.stop) | ||
|
|
||
| self.assertEqual(0, mock.call_count) |
There was a problem hiding this comment.
This assertion is pretty useless: there's no chance for anyone to call threading.Thread.start between the time that you start the mock and this check. Might as well remove.
|
+1, we now test those 2 cases. |
This change makes the projector plugin start a separate thread to compute whether it is active. This allows the
is_activemethod to return immediately. Otherwise, it may take a long time to return, which could cause issues. See #301.We can actually compute the
_is_activeproperty in less time by halting the other thread when we know we have 1 entry in the config object (instead of computing all the values of the object), but computing the config object should usually not take severely long anyway so the added complexity and duplicated logic does not seem worth it.