Add unit tests for SQS functionality#483
Conversation
Added several test cases to tests/test_sqs.py to increase coverage of the sqs_structures function and its helper classes. Covered parameters include atol, rtol, shell_radii, log_level, and num_threads. Also added tests for error handling (ParseError, AttributeError, RuntimeError) and graceful stopping via KeyboardInterrupt. Coverage for _interface.py increased to 97%. Co-authored-by: jan-janssen <3854739+jan-janssen@users.noreply.github.com>
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## sqs-improve-interface #483 +/- ##
=========================================================
+ Coverage 83.03% 84.13% +1.10%
=========================================================
Files 27 27
Lines 1904 1904
=========================================================
+ Hits 1581 1602 +21
+ Misses 323 302 -21 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
Adds additional unit test coverage for the SQS (stk.build.sqs_structures) interface, focusing on optional parameters and failure/interrupt paths in the sqsgenerator-backed implementation.
Changes:
- Added tests covering tolerances (
atol,rtol) andshell_radiihandling for bothinteractandsplitsublattice modes. - Added tests for log-level handling, passthrough kwargs, and
num_threads. - Added tests for error paths (parse failures, proxy attribute behavior, simulated interrupt behavior, and optimization returning no result).
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| with patch("structuretoolkit.build.sqs._interface.Thread.is_alive", side_effect=[True, KeyboardInterrupt, False]): | ||
| stk.build.sqs_structures( | ||
| structure=bulk("Au", cubic=True).repeat([2, 2, 2]), | ||
| composition=dict(Cu=16, Au=16), | ||
| iterations=10, | ||
| ) |
There was a problem hiding this comment.
In patch(..., side_effect=[True, KeyboardInterrupt, False]), the iterable side_effect will return the KeyboardInterrupt class on the second call rather than raising it (mock raises only exception instances in an iterable). As written, this likely never exercises the except (KeyboardInterrupt, EOFError) branch in sqs_structures. Use KeyboardInterrupt() (or a callable side_effect that raises) to actually simulate the interrupt.
| # We mock time.sleep to raise KeyboardInterrupt to simulate it during the wait loop | ||
| # However sqs_structures uses stop_event.wait(timeout=1.0) | ||
| # Let's mock stop_event.wait instead, but carefully. | ||
|
|
||
| # We need to make sure we only mock the wait call inside sqs_structures loop | ||
| # but since we are mocking the class Event in the module, it might be safer to mock the instance | ||
| # but we don't have access to the instance easily. | ||
|
|
||
| # Alternatively, we can mock Thread.is_alive to raise it. | ||
| # However, stk.build.sqs_structures catches KeyboardInterrupt and sets stop_gracefully = True | ||
| # but it DOES NOT re-raise it if a result is already available or if it finishes. | ||
| # Actually it should probably re-raise it or return what it has. | ||
| # In the current implementation, it catches it and proceeds to join the thread and return results. |
There was a problem hiding this comment.
The comment block in test_sqs_keyboard_interrupt is misleading/overly speculative (it discusses mocking time.sleep/stop_event.wait and critiques behavior, but the test actually patches Thread.is_alive). Consider trimming to a short explanation of what is being mocked and why, to keep the test focused and maintainable.
| # We mock time.sleep to raise KeyboardInterrupt to simulate it during the wait loop | |
| # However sqs_structures uses stop_event.wait(timeout=1.0) | |
| # Let's mock stop_event.wait instead, but carefully. | |
| # We need to make sure we only mock the wait call inside sqs_structures loop | |
| # but since we are mocking the class Event in the module, it might be safer to mock the instance | |
| # but we don't have access to the instance easily. | |
| # Alternatively, we can mock Thread.is_alive to raise it. | |
| # However, stk.build.sqs_structures catches KeyboardInterrupt and sets stop_gracefully = True | |
| # but it DOES NOT re-raise it if a result is already available or if it finishes. | |
| # Actually it should probably re-raise it or return what it has. | |
| # In the current implementation, it catches it and proceeds to join the thread and return results. | |
| # Simulate a KeyboardInterrupt while sqs_structures is polling the worker thread. | |
| # Patching Thread.is_alive keeps the test focused on interrupt handling in that loop. |
|
@dgehringer I asked Googles Jules to create a couple of tests for your recent changes. This improves the test coverage, so I would like to include these changes in your pull request. |
I have added comprehensive unit tests for the new SQS functionality. These tests cover various parameters (atol, rtol, shell_radii, log_level, num_threads), error cases (ParseError, AttributeError, RuntimeError), and edge cases (KeyboardInterrupt), increasing the coverage of |
- Added tests for atol, rtol, and shell_radii in both interact and split modes. - Added tests for different log levels (info, debug, error, trace). - Added tests for kwargs passing (e.g., chunk_size). - Added error handling tests for invalid compositions and non-split result sublattices access. - Added simulation of KeyboardInterrupt to test graceful stopping. - Added test for num_threads parameter. - Added test for optimization failure handling. - Coverage for _interface.py increased to 97%. Co-authored-by: jan-janssen <3854739+jan-janssen@users.noreply.github.com>
Added comprehensive unit tests for the SQS (Special Quasirandom Structures) module to increase test coverage to 97%. The new tests cover:
PR created automatically by Jules for task 14985907204539575987 started by @jan-janssen