fix(79): Ensure tests supposed to fail also fail, when monitoring is disabled (--no-monitor cmd flag) #81
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
disabled (--no-monitor cmd flag) changelog updated in this commit too The issue was an exception not being raised when calling wrapped_function when monitoring is disabled in line 216. The raise is being handled now in the following lines 218ff.
7 tasks
|
|
I'm not sure what SonarClouds wants to say with DuplicationOnNewCode or how I could resolve that. |
lhpt2
changed the title
added 3 commits
July 3, 2024 15:49
Returning exceptions inside the wrapped_function() can lead to assertions being ignored, therefore they need to be raised instantly and parent calls need to wrap their call in a try except block.
The memory profiler only reports Exceptions when it should report all Exceptions that inherit from BaseExceptions. This is ultimately reworked in a PR incorporating a simplified memory profiler module into the codebase that fixes not only this issue but also gives the possibility of getting measurements for failing tests. This workaround uses return values to work around the issue. This way pytest-monitor can check for BaseExceptions and act accordingly. Like described earlier the ultimate goal should probably be to replace the whole memory profiler as proposed in this PR: CFMTech#82
|
The newest commit is a bug fix introduced by the memory_profiler module, which doesn't catch all Exceptions (only checks for Exception not BaseException). Worked around this by returning the exception from the wrapped function. This is only a temporary workaround to make everything work, it should be considered to do it as in PR #82 and replace the whole memory profiler module in some way and fix the issues that way. The bug occurs on test runs and causes them to not finish but block execution from finishing. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
There was a bug that resulted in some tests not failing when the cmd flag
--no-monitoris set. Raising a BaseException in the corresponding code that runs when monitoring is disabled (line 218 following) solves the issue.Fixes #79
fix(79): Ensure test supposed to fail also fail, when monitoring is disabled (--no-monitor cmd flag)
changelog updated in this commit too
The issue was an exception not being raised when calling
wrapped_function when monitoring is disabled in line 216.
The raise is being handled now in the following lines 218ff.
Checklist:
- [ ] I have commented my code, particularly in hard-to-understand areas- [ ] I have made corresponding changes to the documentation- [ ] I have added tests that prove my fix is effective or that my feature works- [] New and existing unit tests pass locally with my changes (not just the CI)- [ ] Any dependent changes have been merged and published in downstream modulescreate one !)
Status: Code Review Needed,Business: Test NeededorStatus: In Progressif you are still working on the PR)