Add tests for function crashes #802
Conversation
WalkthroughAdds tests in Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Tester
participant SNE as SingleNodeExecutor
participant Worker
participant Future
Tester->>SNE: submit(exit_funct)
SNE->>Worker: start task
Worker->>Worker: exit_funct() calls sys.exit()
Worker--xSNE: connection closes (socket error)
SNE->>Future: mark as failed with ExecutorlibSocketError
Tester->>Future: result()
Future-->>Tester: raise ExecutorlibSocketError
sequenceDiagram
autonumber
actor Tester
participant SNE as SingleNodeExecutor(init_function=exit_funct)
participant Worker
participant Future
Tester->>SNE: create executor
SNE->>Worker: run init_function
Worker->>Worker: exit_funct() calls sys.exit()
Worker--xSNE: connection closes (socket error)
SNE-->>Tester: subsequent submit/result raises ExecutorlibSocketError
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #802 +/- ##
=======================================
Coverage 97.75% 97.75%
=======================================
Files 32 32
Lines 1468 1468
=======================================
Hits 1435 1435
Misses 33 33 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
tests/test_singlenodeexecutor_noblock.py (2)
26-29: Prefer a non-zero or abrupt exit to better simulate a crashsys.exit() defaults to a clean exit (code 0). If you intend to simulate a crash/abrupt worker termination, exit with a non-zero code or use os._exit(1).
Apply one of these diffs:
-def exit_funct(): - import sys - sys.exit() +def exit_funct(): + import sys + sys.exit(1)or (stronger simulation of a hard crash):
-def exit_funct(): - import sys - sys.exit() +def exit_funct(): + import os + os._exit(1) # abrupt termination; skips atexit handlers
151-169: Flatten nested with statements and drop prints (SIM117); keep the focus on result() raising
- Combine contexts per Ruff SIM117 for brevity.
- print(f.result()) never executes (argument evaluation raises first) and adds noise.
Apply:
class TestFunctionCrashes(unittest.TestCase): def test_single_node_executor(self): - with self.assertRaises(ExecutorlibSocketError): - with SingleNodeExecutor() as exe: - f = exe.submit(exit_funct) - print(f.result()) + with self.assertRaises(ExecutorlibSocketError), SingleNodeExecutor() as exe: + exe.submit(exit_funct).result() def test_single_node_executor_block_allocation(self): - with self.assertRaises(ExecutorlibSocketError): - with SingleNodeExecutor(block_allocation=True) as exe: - f = exe.submit(exit_funct) - print(f.result()) + with self.assertRaises(ExecutorlibSocketError), SingleNodeExecutor(block_allocation=True) as exe: + exe.submit(exit_funct).result() def test_single_node_executor_init_function(self): - with self.assertRaises(ExecutorlibSocketError): - with SingleNodeExecutor(init_function=exit_funct, block_allocation=True) as exe: - f = exe.submit(sum, [1, 1]) - print(f.result()) + with self.assertRaises(ExecutorlibSocketError), SingleNodeExecutor(init_function=exit_funct, block_allocation=True) as exe: + exe.submit(sum, [1, 1]).result()(optional) If serialization ever flakes on some platforms, mirror other tests and call cloudpickle_register() before submit.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
tests/test_singlenodeexecutor_noblock.py(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
tests/test_singlenodeexecutor_noblock.py (1)
executorlib/standalone/interactive/communication.py (1)
ExecutorlibSocketError(10-11)
🪛 Ruff (0.12.2)
tests/test_singlenodeexecutor_noblock.py
153-154: Use a single with statement with multiple contexts instead of nested with statements
(SIM117)
159-160: Use a single with statement with multiple contexts instead of nested with statements
(SIM117)
165-166: Use a single with statement with multiple contexts instead of nested with statements
(SIM117)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (14)
- GitHub Check: unittest_openmpi (ubuntu-22.04-arm, 3.13)
- GitHub Check: unittest_openmpi (ubuntu-latest, 3.13)
- GitHub Check: unittest_openmpi (ubuntu-latest, 3.12)
- GitHub Check: unittest_openmpi (ubuntu-latest, 3.11)
- GitHub Check: unittest_win
- GitHub Check: unittest_openmpi (ubuntu-24.04-arm, 3.13)
- GitHub Check: unittest_openmpi (macos-latest, 3.13)
- GitHub Check: unittest_mpich (macos-latest, 3.13)
- GitHub Check: benchmark (ubuntu-latest, 3.13, .ci_support/environment-mpich.yml)
- GitHub Check: notebooks_integration
- GitHub Check: unittest_flux_mpich
- GitHub Check: unittest_flux_openmpi
- GitHub Check: benchmark (ubuntu-latest, 3.13, .ci_support/environment-openmpi.yml)
- GitHub Check: unittest_slurm_mpich
🔇 Additional comments (2)
tests/test_singlenodeexecutor_noblock.py (2)
6-6: Good: asserting on the precise error typeImporting ExecutorlibSocketError enables clear, specific assertions in the new tests.
148-149: Confirm determinism of worker-id expectation under block_allocation=FalseAsserting sum(...) == 0 assumes both tasks always run on worker 0. If scheduling can vary across platforms/backends, this may flake. Consider asserting “same worker” rather than the specific id.
If needed, use:
- self.assertEqual(sum([f1_worker_id.result(), f2_worker_id.result()]), 0) + w1, w2 = f1_worker_id.result(), f2_worker_id.result() + self.assertEqual(w1, w2) # both tasks handled by the same worker + self.assertIn(w1, (0, 1)) # allow backend-specific numbering
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
tests/test_singlenodeexecutor_noblock.py (5)
26-29: Make the simulated crash explicitly abnormal (non-zero exit).Using
sys.exit(1)better reflects a crash and avoids ambiguity with a clean exit status.-def exit_funct(): - import sys - sys.exit() +def exit_funct(): + import sys + sys.exit(1)
152-157: Remove print noise, add timeout, and merge context managers.Prevents noisy test output, avoids hangs, and satisfies SIM117.
- with self.assertRaises(ExecutorlibSocketError): - with SingleNodeExecutor(max_workers=2) as exe: - f = exe.submit(exit_funct) - print(f.result()) + with self.assertRaises(ExecutorlibSocketError), SingleNodeExecutor(max_workers=2) as exe: + f = exe.submit(exit_funct) + _ = f.result(timeout=10)
159-163: Same cleanups for the block-allocation case.- with self.assertRaises(ExecutorlibSocketError): - with SingleNodeExecutor(max_workers=2, block_allocation=True) as exe: - f = exe.submit(exit_funct) - print(f.result()) + with self.assertRaises(ExecutorlibSocketError), SingleNodeExecutor(max_workers=2, block_allocation=True) as exe: + f = exe.submit(exit_funct) + _ = f.result(timeout=10)
165-169: Apply the same pattern for the init_function path.Keeps tests consistent, quieter, and less likely to hang.
- with self.assertRaises(ExecutorlibSocketError): - with SingleNodeExecutor(max_workers=2, init_function=exit_funct, block_allocation=True) as exe: - f = exe.submit(sum, [1, 1]) - print(f.result()) + with self.assertRaises(ExecutorlibSocketError), SingleNodeExecutor( + max_workers=2, init_function=exit_funct, block_allocation=True + ) as exe: + f = exe.submit(sum, [1, 1]) + _ = f.result(timeout=10)
151-169: De-duplicate the three crash tests.Minor: factor a tiny helper to reduce repetition and speed future changes.
Example (outside these lines, for illustration):
def _expect_socket_error(self, **kwargs): with self.assertRaises(ExecutorlibSocketError), SingleNodeExecutor(**kwargs) as exe: f = exe.submit(exit_funct if 'init_function' not in kwargs else sum, [1,1]) if 'init_function' in kwargs else exe.submit(exit_funct) _ = f.result(timeout=10)Then call it from each test with the respective kwargs.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
tests/test_singlenodeexecutor_noblock.py(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
tests/test_singlenodeexecutor_noblock.py (3)
executorlib/standalone/interactive/communication.py (1)
ExecutorlibSocketError(10-11)executorlib/executor/single.py (1)
SingleNodeExecutor(20-189)executorlib/task_scheduler/interactive/blockallocation.py (3)
max_workers(77-78)max_workers(81-103)submit(105-141)
🪛 Ruff (0.12.2)
tests/test_singlenodeexecutor_noblock.py
153-154: Use a single with statement with multiple contexts instead of nested with statements
(SIM117)
159-160: Use a single with statement with multiple contexts instead of nested with statements
(SIM117)
165-166: Use a single with statement with multiple contexts instead of nested with statements
(SIM117)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (18)
- GitHub Check: unittest_win
- GitHub Check: benchmark (ubuntu-latest, 3.13, .ci_support/environment-openmpi.yml)
- GitHub Check: benchmark (ubuntu-latest, 3.13, .ci_support/environment-mpich.yml)
- GitHub Check: unittest_slurm_mpich
- GitHub Check: unittest_flux_openmpi
- GitHub Check: notebooks_integration
- GitHub Check: unittest_mpich (macos-latest, 3.13)
- GitHub Check: unittest_mpich (ubuntu-24.04-arm, 3.13)
- GitHub Check: unittest_mpich (ubuntu-latest, 3.12)
- GitHub Check: unittest_mpich (ubuntu-latest, 3.11)
- GitHub Check: unittest_mpich (ubuntu-latest, 3.13)
- GitHub Check: unittest_openmpi (ubuntu-latest, 3.12)
- GitHub Check: unittest_openmpi (ubuntu-latest, 3.11)
- GitHub Check: unittest_openmpi (ubuntu-22.04-arm, 3.13)
- GitHub Check: unittest_openmpi (ubuntu-latest, 3.13)
- GitHub Check: unittest_openmpi (ubuntu-24.04-arm, 3.13)
- GitHub Check: unittest_openmpi (macos-latest, 3.13)
- GitHub Check: unittest_flux_mpich
🔇 Additional comments (2)
tests/test_singlenodeexecutor_noblock.py (2)
6-6: Import looks correct and scoped.Bringing
ExecutorlibSocketErrorfrom its defining module is appropriate for these tests.
148-149: Double-check determinism of expected worker-id sum.Asserting a strict
0may be brittle if scheduling varies by platform or CI load. Please confirm this is guaranteed by design forblock_allocation=False, max_cores=2; otherwise consider a looser assertion (or document the invariant here).
Summary by CodeRabbit