-
Notifications
You must be signed in to change notification settings - Fork 136
test: enhance testing logging #4097
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
Conversation
Thanks for opening a Pull Request. If you want to perform a review write a comment saying: @ansys-reviewer-bot review |
Reviewer's GuideImproved test logging and debugging capabilities by consolidating logging functions, introducing a DPF backend flag, refining error messages, enhancing MAPDL session lifecycle logs, and increasing fixture verbosity. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
@pyansys-ci-bot LGTM. |
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.
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.
Pull Request Overview
This PR improves test logging by introducing a unified log_test
function with start/end markers, adds a TEST_DPF_BACKEND
flag, and increases the default log level to DEBUG during Mapdl launches.
- Renamed
log_test_start
tolog_test
with an optionalend
parameter and updated its usage. - Added a
test_dpf_backend
helper andTEST_DPF_BACKEND
constant to toggle DPF backend tests. - Changed default Mapdl launch loglevel from ERROR to DEBUG.
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
File | Description |
---|---|
tests/test_mapdl.py | Imported new flags (IS_SMP , ON_CI , ON_LOCAL , QUICK_LAUNCH_SWITCHES , requires ) and removed a duplicate import |
tests/conftest.py | Replaced log_test_start with log_test , added TEST_DPF_BACKEND , enhanced error handling in get_error_message , and updated loglevel |
tests/common.py | Added the test_dpf_backend function to parse TEST_DPF_BACKEND env var |
Comments suppressed due to low confidence (3)
tests/common.py:172
- [nitpick] The new
test_dpf_backend
function lacks a docstring. Please add a brief description explaining its purpose and expected inputs/outputs.
def test_dpf_backend() -> bool:
tests/conftest.py:64
- You import
IS_SMP
intests/test_mapdl.py
butconftest.py
doesn’t define anIS_SMP
constant. Consider addingIS_SMP = is_smp()
alongside the other environment-based constants.
TEST_DPF_BACKEND = test_dpf_backend()
tests/conftest.py:403
- Raising a generic exception for unknown
longrepr
types may abort the entire test suite unexpectedly. Consider falling back to a safe default message or logging a warning instead of throwing.
raise Exception(str(rep.longrepr))
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.
Hey @germa89 - I've reviewed your changes - here's some feedback:
- Rename helper function
test_dpf_backend
to avoid pytest test discovery (e.g.is_dpf_backend_enabled
) to prevent accidental execution as a test. - Consider adding a custom pytest CLI option (via pytest_addoption) to toggle DPF backend tests rather than relying on environment variables for better configurability.
- Split the unified
log_test
function into distinctlog_test_start
andlog_test_end
functions to improve readability and avoid boolean flags.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Rename helper function `test_dpf_backend` to avoid pytest test discovery (e.g. `is_dpf_backend_enabled`) to prevent accidental execution as a test.
- Consider adding a custom pytest CLI option (via pytest_addoption) to toggle DPF backend tests rather than relying on environment variables for better configurability.
- Split the unified `log_test` function into distinct `log_test_start` and `log_test_end` functions to improve readability and avoid boolean flags.
## Individual Comments
### Comment 1
<location> `tests/common.py:223` </location>
<code_context>
return elements
-def log_test_start(mapdl: Mapdl) -> None:
+def log_test(mapdl: Mapdl, end=False) -> None:
"""Print the current test to the MAPDL log file and console output."""
test_name = os.environ.get(
</code_context>
<issue_to_address>
Consider adding or updating tests for the new log_test() function, especially for the 'end' parameter.
Ensure tests cover both start and end logging behaviors, as well as edge cases like long test names and missing environment variables.
</issue_to_address>
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
@pyansys-ci-bot LGTM. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #4097 +/- ##
==========================================
- Coverage 91.82% 91.79% -0.03%
==========================================
Files 187 187
Lines 15033 15033
==========================================
- Hits 13804 13800 -4
- Misses 1229 1233 +4 🚀 New features to boost your workflow:
|
Description
As the title. Improve logging while testing.
Issue linked
NA but related to #1300.
Checklist
draft
if it is not ready to be reviewed yet.feat: adding new MAPDL command
)Summary by Sourcery
Improve testing logging and debugging, introduce controlled DPF backend testing, and adjust test fixtures for enhanced output and error handling.
New Features:
Enhancements:
Tests: