-
Notifications
You must be signed in to change notification settings - Fork 84
ci: fast CI #207
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
ci: fast CI #207
Changes from 9 commits
2e6a4a4
0abc519
0ac2256
15e6911
d380c4b
816c214
0785e06
2a9e4f2
f07f335
6a51f51
f649f7b
35a7381
e021617
f19187b
dd73fd4
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,7 +2,7 @@ | |
| Copyright 2025 Amazon.com, Inc. or its affiliates. All Rights Reserved. | ||
| """ | ||
|
|
||
| import multiprocessing | ||
| import threading | ||
| import unittest | ||
| from unittest.mock import patch, MagicMock | ||
|
|
||
|
|
@@ -18,33 +18,41 @@ def setUp(self): | |
| self.socket = "/tmp/sock" | ||
|
|
||
| def test_success_and_failure_isolation(self): | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this test needs to be updated to reflect what are actually checking, using fork explicitly hides what's actually being used, so in the context of python 3.14 we force the test to use fork while we know the default now is to use forkserver. I'd use multiprocessing.Manager to track the shared data and this way we won't need to force usage of fork
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ok, will change this |
||
| success_counter = multiprocessing.Value("i", 0) | ||
| fail_counter = multiprocessing.Value("i", 0) | ||
| success_counter = 0 | ||
| fail_counter = 0 | ||
| process_index = 0 | ||
| lock = threading.Lock() | ||
|
|
||
| def fake_bootstrap_run(handler, lambda_runtime_client): | ||
| pid = multiprocessing.current_process().pid | ||
| if pid % 2 == 0: | ||
| nonlocal success_counter, fail_counter, process_index | ||
| with lock: | ||
| idx = process_index | ||
| process_index += 1 | ||
| if idx % 2 == 0: | ||
| for _ in range(3): | ||
| with success_counter.get_lock(): | ||
| success_counter.value += 1 | ||
| with lock: | ||
| success_counter += 1 | ||
| else: | ||
| with fail_counter.get_lock(): | ||
| fail_counter.value += 1 | ||
| with lock: | ||
| fail_counter += 1 | ||
| raise RuntimeError("Simulated failure") | ||
|
|
||
| with patch( | ||
| "awslambdaric.lambda_multi_concurrent_utils.MultiConcurrentRunner._redirect_output" | ||
| ), patch( | ||
| "awslambdaric.lambda_multi_concurrent_utils.bootstrap.run", | ||
| side_effect=fake_bootstrap_run, | ||
| ), patch( | ||
| "awslambdaric.lambda_multi_concurrent_utils.multiprocessing.Process", | ||
| threading.Thread, | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What i meant is that we should mock this at all, and rely on multiprocessing manager to track shared data, so here we're testing something different
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I see, I've changed back to no mock, but I still needed to add |
||
| ): | ||
| # spawn 4 multi-concurrent processes | ||
| MultiConcurrentRunner.run_concurrent( | ||
| self.handler, self.addr, self.use_thread, self.socket, max_concurrency=4 | ||
| ) | ||
|
|
||
| self.assertEqual(success_counter.value, 6) | ||
| self.assertEqual(fail_counter.value, 2) | ||
| self.assertEqual(success_counter, 6) | ||
| self.assertEqual(fail_counter, 2) | ||
|
|
||
|
|
||
| if __name__ == "__main__": | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is repeated across all jobs, having it in a common job would reduce time further
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
true, but this is going away with #208 so if that looks good to you let's merge both?