Skip to content

Commit

Permalink
Fix failing tests of hid.write on MacOS by pulling target functions t…
Browse files Browse the repository at this point in the history
…o top of module (#1344)

Resolves #1343.

On MacOS, the tests of `hid.write.ProcessWithResult` fail because the
multiprocessing module tries to pickle the inlined target functions so
it can then spawn child processes. To be picklable, functions need to be
defined with `def` in the top level of a
module[[1](https://docs.python.org/3/library/pickle.html#what-can-be-pickled-and-unpickled)].
On MacOS, multiprocessing uses spawn instead of
fork[[2](python/cpython@17a5588)],
so this is a problem that does not show up on
Linux[[3](https://docs.python.org/3.11/library/multiprocessing.html#the-spawn-and-forkserver-start-methods)].

After moving the definitions of these functions to the top level of the
module, the tests pass both on MacOS and on Linux running on CircleCI.

<a data-ca-tag
href="https://codeapprove.com/pr/tiny-pilot/tinypilot/1344"><img
src="https://codeapprove.com/external/github-tag-allbg.png" alt="Review
on CodeApprove" /></a>

---------

Co-authored-by: Andrew M. Farrell <armorsmith42@gmail.com>
  • Loading branch information
amfarrell and Andrew M. Farrell authored Mar 30, 2023
1 parent 4ae349b commit 1371487
Showing 1 changed file with 35 additions and 20 deletions.
55 changes: 35 additions & 20 deletions app/hid/write_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -5,15 +5,40 @@

import hid.write

# Dummy functions to represent what can happen when a Human Interface Device
# writes.
#
# On some MacOS systems, the multiprocessing module spawns rather than forks new
# processes[1], which pickles these functions[2]. So, they must be defined
# using `def` at the top level of a module[3].
#
# This was observed on a 2021 Macbook Pro M1 Max running OSX Ventura 13.2.1.
#
# [1] https://github.com/python/cpython/commit/17a5588740b3d126d546ad1a13bdac4e028e6d50
# [2] https://docs.python.org/3.11/library/multiprocessing.html#the-spawn-and-forkserver-start-methods
# [3] https://docs.python.org/3/library/pickle.html#what-can-be-pickled-and-unpickled:~:text=(using%20def%2C%20not%20lambda)

class WriteTest(unittest.TestCase):

def test_process_with_result_child_completed(self):
def do_nothing():
pass


def sleep_1_second():
time.sleep(1)


def target():
pass
def raise_exception():
raise Exception('Child exception')

process = hid.write.ProcessWithResult(target=target, daemon=True)

def return_string():
return 'Done!'


class WriteTest(unittest.TestCase):

def test_process_with_result_child_completed(self):
process = hid.write.ProcessWithResult(target=do_nothing, daemon=True)
process.start()
process.join()
result = process.result()
Expand All @@ -22,11 +47,8 @@ def target():
hid.write.ProcessResult(return_value=None, exception=None), result)

def test_process_with_result_child_not_completed(self):

def target():
time.sleep(1)

process = hid.write.ProcessWithResult(target=target, daemon=True)
process = hid.write.ProcessWithResult(target=sleep_1_second,
daemon=True)
process.start()
# Get the result before the child process has completed.
self.assertIsNone(process.result())
Expand All @@ -35,14 +57,11 @@ def target():
process.kill()

def test_process_with_result_child_exception(self):

def target():
raise Exception('Child exception')

# Silence stderr while the child exception is being raised to avoid
# polluting the terminal output.
with mock.patch('sys.stderr', io.StringIO()):
process = hid.write.ProcessWithResult(target=target, daemon=True)
process = hid.write.ProcessWithResult(target=raise_exception,
daemon=True)
process.start()
process.join()
result = process.result()
Expand All @@ -53,11 +72,7 @@ def target():
self.assertEqual('Child exception', str(result.exception))

def test_process_with_result_return_value(self):

def target():
return 'Done!'

process = hid.write.ProcessWithResult(target=target, daemon=True)
process = hid.write.ProcessWithResult(target=return_string, daemon=True)
process.start()
process.join()
result = process.result()
Expand Down

0 comments on commit 1371487

Please sign in to comment.