map(): Evaluate generator within with-statement

[jan-janssen]

Summary by CodeRabbit

• Tests
  • Standardized test behavior by eagerly materializing map results, simplifying assertions across multiple executor and spawner scenarios (serial, multi-core, working directory, and single-node cases).
  • Improves test consistency and readability without altering functional outcomes.
  • Confirms no changes to public APIs or runtime behavior.

[coderabbitai]

Walkthrough

Tests were updated to eagerly materialize map results with list(p.map(...)) and to compare the resulting lists directly in assertions. No production code or public APIs were modified.

Changes

Cohort / File(s)	Summary of changes
Eager materialization in tests tests/test_fluxjobexecutor.py, tests/test_fluxpythonspawner.py, tests/test_mpiexecspawner.py, tests/test_singlenodeexecutor_mpi.py	Replaced lazy map usage with eager list(p.map(...)); updated assertions to compare the materialized list directly instead of wrapping again with list(). No functional or API changes.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~8 minutes

Poem

I leapt through lists with gentle grace,
Turning maps to baskets, just in case.
No APIs moved, no engines stirred—
Only tests that now prefer the herd.
Thump-thump! My paws approve this feat,
Eager carrots, results complete. 🥕🐇

✨ Finishing Touches

• 📝 Generate Docstrings

🧪 Generate unit tests

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch map_fixes

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 97.76%. Comparing base (3524942) to head (85d8f83).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main     #812   +/-   ##
=======================================
  Coverage   97.76%   97.76%           
=======================================
  Files          32       32           
  Lines        1479     1479           
=======================================
  Hits         1446     1446           
  Misses         33       33           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[coderabbitai]

Actionable comments posted: 0

🧹 Nitpick comments (2)

tests/test_fluxjobexecutor.py (2)

125-129: Optional: make log-file cleanup resilient to assertion failures.

If an assertion fails, the remove(...) calls won’t run. Consider registering cleanup handlers instead:

# near file_stdout/file_stderr definitions
self.addCleanup(lambda: os.path.exists(file_stdout) and os.remove(file_stdout))
self.addCleanup(lambda: os.path.exists(file_stderr) and os.remove(file_stderr))

---

145-149: Same optional cleanup robustness here.

Use addCleanup (or a try/finally) so log files are removed even if assertions fail.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 3524942 and 85d8f83.

📒 Files selected for processing (4)

• tests/test_fluxjobexecutor.py (3 hunks)
• tests/test_fluxpythonspawner.py (1 hunks)
• tests/test_mpiexecspawner.py (2 hunks)
• tests/test_singlenodeexecutor_mpi.py (1 hunks)

🧰 Additional context used

🧬 Code graph analysis (3)

tests/test_fluxpythonspawner.py (1)

tests/test_fluxjobexecutor.py (1)

• mpi_funct (23-28)

tests/test_mpiexecspawner.py (1)

tests/test_fluxjobexecutor.py (1)

• mpi_funct (23-28)

tests/test_fluxjobexecutor.py (1)

tests/test_fluxpythonspawner.py (2)

• mpi_funct (26-31)
• calc (22-23)

⏰ 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). (16)

• GitHub Check: notebooks_integration
• GitHub Check: benchmark (ubuntu-latest, 3.13, .ci_support/environment-mpich.yml)
• GitHub Check: benchmark (ubuntu-latest, 3.13, .ci_support/environment-openmpi.yml)
• GitHub Check: notebooks
• GitHub Check: unittest_openmpi (ubuntu-24.04-arm, 3.13)
• GitHub Check: unittest_openmpi (ubuntu-latest, 3.11)
• GitHub Check: unittest_openmpi (ubuntu-latest, 3.13)
• GitHub Check: unittest_mpich (ubuntu-latest, 3.12)
• GitHub Check: unittest_openmpi (macos-latest, 3.13)
• GitHub Check: unittest_mpich (macos-latest, 3.13)
• GitHub Check: unittest_old
• GitHub Check: unittest_mpich (ubuntu-latest, 3.13)
• GitHub Check: unittest_slurm_mpich
• GitHub Check: unittest_mpich (ubuntu-22.04-arm, 3.13)
• GitHub Check: unittest_win
• GitHub Check: unittest_flux_openmpi

🔇 Additional comments (6)

tests/test_fluxpythonspawner.py (1)

102-105: Eager materialization inside the context is correct.

Consuming the map iterator with list(...) before the with-block exits ensures resources from FluxPythonSpawner remain valid during iteration. Good fix.

tests/test_singlenodeexecutor_mpi.py (1)

122-126: Good change: evaluate map within the executor context.

Materializing results before teardown avoids using a closed pool/spawner.

tests/test_fluxjobexecutor.py (1)

107-111: Solid: list(...) ensures generator is evaluated while FluxJobExecutor is alive.

tests/test_mpiexecspawner.py (3)

326-327: Correct: consume map results within the with-statement.

Prevents iterating after the scheduler has shut down.

---

433-437: Good: early materialization for the MPI map case as well.

---

433-437: Approve changes — no remaining lazy map usages detected
Sanity check script found no .map(…) calls outside of list()/tuple() wrappers in the test suite.