test(context): add coverage for WorkContext#1076
test(context): add coverage for WorkContext#1076mnadzam wants to merge 1 commit intopython-wheel-build:mainfrom
Conversation
📝 WalkthroughWalkthroughAdds extensive unit tests for WorkContext in tests/test_context.py. Introduces Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/test_context.py`:
- Around line 174-181: After asserting ctx.clean_build_dirs raises, add
assertions to verify the safety check did not remove directories: assert that
sdist_root.exists() and build_env.path.exists() (and optionally that their
contents remain intact) after the pytest.raises block. Locate the test using
sdist_root, build_env, and the call to ctx.clean_build_dirs and add these
post-raise existence checks to ensure no delete-then-raise regression can pass.
- Around line 108-114: The test currently only checks that
tmp_context.wheels_build is a child of tmp_context.wheels_build_base; instead,
exercise the thread-specific behavior by creating a second thread that calls
tmp_context.wheels_build and captures its value, then assert that the path
returned in the worker thread is different from the main thread's
tmp_context.wheels_build and that both still share the same
tmp_context.wheels_build_base; update test_wheels_build_parallel to spawn a
worker thread (or use threading.Thread), store the worker_path, join the thread,
and add an assertion comparing worker_path != main_thread_path while keeping the
existing parent/is_dir assertions on the appropriate values.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: bb0752cf-2ded-4366-904f-420e1efc1eeb
📒 Files selected for processing (1)
tests/test_context.py
There was a problem hiding this comment.
♻️ Duplicate comments (1)
tests/test_context.py (1)
108-114:⚠️ Potential issue | 🟡 MinorExercise an actual worker thread here.
This still only proves
wheels_buildreturns a child directory; it would pass if every thread got the same path. Capturewheels_buildfrom a second thread and assert it differs from the main thread path.Suggested test strengthening
+import threading from unittest.mock import Mock, patchdef test_wheels_build_parallel(tmp_context: context.WorkContext) -> None: tmp_context.enable_parallel_builds() - result = tmp_context.wheels_build + main_thread_path = tmp_context.wheels_build + worker_paths: list[pathlib.Path] = [] + + def capture_worker_path() -> None: + worker_paths.append(tmp_context.wheels_build) + + worker = threading.Thread(target=capture_worker_path) + worker.start() + worker.join() - assert result.parent == tmp_context.wheels_build_base - assert result.is_dir() + assert main_thread_path.parent == tmp_context.wheels_build_base + assert worker_paths[0].parent == tmp_context.wheels_build_base + assert worker_paths[0] != main_thread_path + assert main_thread_path.is_dir() + assert worker_paths[0].is_dir()As per coding guidelines,
tests/**: “Verify test actually tests the intended behavior. Check for missing edge cases.”🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_context.py` around lines 108 - 114, The test test_wheels_build_parallel currently only checks that tmp_context.wheels_build is a child directory; to exercise parallel behavior spawn a second worker thread (e.g., using threading.Thread or concurrent.futures) after calling tmp_context.enable_parallel_builds(), have that thread call tmp_context.wheels_build and return or store its path, then assert the second thread’s path is not equal to the main thread’s tmp_context.wheels_build and that both exist; locate symbols test_wheels_build_parallel, tmp_context.enable_parallel_builds(), and tmp_context.wheels_build to implement this change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@tests/test_context.py`:
- Around line 108-114: The test test_wheels_build_parallel currently only checks
that tmp_context.wheels_build is a child directory; to exercise parallel
behavior spawn a second worker thread (e.g., using threading.Thread or
concurrent.futures) after calling tmp_context.enable_parallel_builds(), have
that thread call tmp_context.wheels_build and return or store its path, then
assert the second thread’s path is not equal to the main thread’s
tmp_context.wheels_build and that both exist; locate symbols
test_wheels_build_parallel, tmp_context.enable_parallel_builds(), and
tmp_context.wheels_build to implement this change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e26a76c7-cf0f-4924-919a-e3103cf7447f
📒 Files selected for processing (1)
tests/test_context.py
rd4398
left a comment
rd4398
left a comment
There was a problem hiding this comment.
Overall looks good, I have added a comment
Can you please add detailed commit message? Just "closes xyz" is not enough. Adding a detailed commit message helps when we look at git history.
- Add dedicated unit tests for setup(), package_build_info(), enable_parallel_builds(), wheels_build, pip_wheel_server_args, uv_clean_cache(), write_to_graph_to_file(), and clean_build_dirs() in context.py. - Decompose shared setup into _make_context() and _all_setup_dirs(). - Increase unit test coverage of context.py from 67% to 94%. Closes python-wheel-build#1075 Signed-off-by: Marcel Nadzam <mnadzam@redhat.com> Co-authored-by: Cursor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
tests/test_context.py (1)
3-3:⚠️ Potential issue | 🟡 MinorExercise
wheels_buildfrom a second thread.This still only checks that the main-thread path is a child of
wheels_build_base; it would not catch a regression where all threads share the same build directory.Suggested test strengthening
import os import pathlib +import threading from unittest.mock import Mock, patchdef test_wheels_build_parallel(tmp_context: context.WorkContext) -> None: tmp_context.enable_parallel_builds() result = tmp_context.wheels_build + worker_results: list[pathlib.Path] = [] + + def capture_worker_result() -> None: + worker_results.append(tmp_context.wheels_build) + + worker = threading.Thread(target=capture_worker_result) + worker.start() + worker.join() assert result.parent == tmp_context.wheels_build_base + assert worker_results[0].parent == tmp_context.wheels_build_base + assert worker_results[0] != result assert result.is_dir() + assert worker_results[0].is_dir()As per coding guidelines,
tests/**: “Verify test actually tests the intended behavior. Check for missing edge cases.”Also applies to: 108-114
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_context.py` at line 3, Add a second-thread execution of the wheels_build path to ensure threads get distinct build dirs: in the test (e.g., test_wheels_build_thread_isolation) spawn a new Thread that calls wheels_build() while the main thread also calls wheels_build(); then assert both returned build directories are children of wheels_build_base and are not equal, referencing the wheels_build function and wheels_build_base constant to locate the code to update.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@tests/test_context.py`:
- Line 3: Add a second-thread execution of the wheels_build path to ensure
threads get distinct build dirs: in the test (e.g.,
test_wheels_build_thread_isolation) spawn a new Thread that calls wheels_build()
while the main thread also calls wheels_build(); then assert both returned build
directories are children of wheels_build_base and are not equal, referencing the
wheels_build function and wheels_build_base constant to locate the code to
update.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 41c9c08b-de80-412a-a8bc-897563a5d418
📒 Files selected for processing (1)
tests/test_context.py
|
I have added a more detailed commit message. Is it good like this? Or should it also describe each test? |
2 participants
setup(),package_build_info(),enable_parallel_builds(),wheels_build,pip_wheel_server_args,uv_clean_cache(),write_to_graph_to_file(), andclean_build_dirs()incontext.py._make_context()and_all_setup_dirs().context.pyfrom 67% to 94%.Closes #1075