From 22ddcd0deffd90fd3e3a519ec542f6140378b232 Mon Sep 17 00:00:00 2001 From: Lucas Haupt Date: Wed, 8 May 2024 14:39:19 +0200 Subject: [PATCH 1/4] 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. --- docs/sources/changelog.rst | 2 +- pytest_monitor/pytest_monitor.py | 4 +++- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/docs/sources/changelog.rst b/docs/sources/changelog.rst index 0b77e9e..5d2616d 100644 --- a/docs/sources/changelog.rst +++ b/docs/sources/changelog.rst @@ -1,7 +1,7 @@ ========= Changelog ========= - +* :bug: `#79` Fix a bug concerning commandline flag `--no-monitor` causing tests that are supposed to fail to pass instead * :release:`1.6.6 <2023-05-06>` * :bug:`#64` Prepare version 1.7.0 of pytest-monitor. Last version to support Python <= 3.7 and all pytest <= 5.* * :bug:`#0` Improve and fix some CI issues, notably one that may cause python to not be the requested one but a more recent one. diff --git a/pytest_monitor/pytest_monitor.py b/pytest_monitor/pytest_monitor.py index 3e9c12c..7eb5a2d 100644 --- a/pytest_monitor/pytest_monitor.py +++ b/pytest_monitor/pytest_monitor.py @@ -213,7 +213,9 @@ def prof(): setattr(pyfuncitem, "monitor_results", True) if not PYTEST_MONITORING_ENABLED: - wrapped_function() + e = wrapped_function() + if isinstance(e, BaseException): + raise e else: if not pyfuncitem.session.config.option.mtr_disable_gc: gc.collect() From b2b437ab643b0f03b8adfabe427065a8b81405ac Mon Sep 17 00:00:00 2001 From: Lucas Haupt Date: Wed, 3 Jul 2024 15:49:00 +0200 Subject: [PATCH 2/4] Changes to fix, raise exception inside wrapped_function(), avoid issues 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. --- pytest_monitor/pytest_monitor.py | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/pytest_monitor/pytest_monitor.py b/pytest_monitor/pytest_monitor.py index 7eb5a2d..7eb14d4 100644 --- a/pytest_monitor/pytest_monitor.py +++ b/pytest_monitor/pytest_monitor.py @@ -202,7 +202,7 @@ def wrapped_function(): except Exception: raise except BaseException as e: - return e + raise e def prof(): m = memory_profiler.memory_usage((wrapped_function, ()), max_iterations=1, max_usage=True, retval=True) @@ -213,9 +213,10 @@ def prof(): setattr(pyfuncitem, "monitor_results", True) if not PYTEST_MONITORING_ENABLED: - e = wrapped_function() - if isinstance(e, BaseException): - raise e + try: + wrapped_function() + except BaseException: + raise else: if not pyfuncitem.session.config.option.mtr_disable_gc: gc.collect() From d831b4f2ea3e11211612b7d8aff51ebc08b663bc Mon Sep 17 00:00:00 2001 From: Lucas Haupt Date: Wed, 3 Jul 2024 18:11:45 +0200 Subject: [PATCH 3/4] Minor fix: Don't raise e, just use raise instead (no var needed here) --- pytest_monitor/pytest_monitor.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/pytest_monitor/pytest_monitor.py b/pytest_monitor/pytest_monitor.py index 7eb14d4..55eb7ae 100644 --- a/pytest_monitor/pytest_monitor.py +++ b/pytest_monitor/pytest_monitor.py @@ -201,8 +201,8 @@ def wrapped_function(): pyfuncitem.obj(**testargs) except Exception: raise - except BaseException as e: - raise e + except BaseException: + raise def prof(): m = memory_profiler.memory_usage((wrapped_function, ()), max_iterations=1, max_usage=True, retval=True) From c645da25354ab64df479fdd43f887cd11888f89c Mon Sep 17 00:00:00 2001 From: Lucas Haupt Date: Mon, 15 Jul 2024 15:47:10 +0200 Subject: [PATCH 4/4] Add workaround fix for faulty memory_profiler module 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: https://github.com/CFMTech/pytest-monitor/pull/82 --- pytest_monitor/pytest_monitor.py | 96 ++++++++++++++++++++++++-------- 1 file changed, 74 insertions(+), 22 deletions(-) diff --git a/pytest_monitor/pytest_monitor.py b/pytest_monitor/pytest_monitor.py index 55eb7ae..0f8a5dc 100644 --- a/pytest_monitor/pytest_monitor.py +++ b/pytest_monitor/pytest_monitor.py @@ -22,7 +22,9 @@ "monitor_test_if": (True, "monitor_force_test", lambda x: bool(x), False), } PYTEST_MONITOR_DEPRECATED_MARKERS = {} -PYTEST_MONITOR_ITEM_LOC_MEMBER = "_location" if tuple(pytest.__version__.split(".")) < ("5", "3") else "location" +PYTEST_MONITOR_ITEM_LOC_MEMBER = ( + "_location" if tuple(pytest.__version__.split(".")) < ("5", "3") else "location" +) PYTEST_MONITORING_ENABLED = True @@ -44,7 +46,9 @@ def pytest_addoption(parser): help="Set this option to distinguish parametrized tests given their values." " This requires the parameters to be stringifiable.", ) - group.addoption("--no-monitor", action="store_true", dest="mtr_none", help="Disable all traces") + group.addoption( + "--no-monitor", action="store_true", dest="mtr_none", help="Disable all traces" + ) group.addoption( "--remote-server", action="store", @@ -68,13 +72,15 @@ def pytest_addoption(parser): "--force-component", action="store", dest="mtr_force_component", - help="Force the component to be set at the given value for the all tests run" " in this session.", + help="Force the component to be set at the given value for the all tests run" + " in this session.", ) group.addoption( "--component-prefix", action="store", dest="mtr_component_prefix", - help="Prefix each found components with the given value (applies to all tests" " run in this session).", + help="Prefix each found components with the given value (applies to all tests" + " run in this session).", ) group.addoption( "--no-gc", @@ -99,10 +105,13 @@ def pytest_addoption(parser): def pytest_configure(config): - config.addinivalue_line("markers", "monitor_skip_test: mark test to be executed but not monitored.") + config.addinivalue_line( + "markers", "monitor_skip_test: mark test to be executed but not monitored." + ) config.addinivalue_line( "markers", - "monitor_skip_test_if(cond): mark test to be executed but " "not monitored if cond is verified.", + "monitor_skip_test_if(cond): mark test to be executed but " + "not monitored if cond is verified.", ) config.addinivalue_line( "markers", @@ -126,14 +135,24 @@ def pytest_runtest_setup(item): """ if not PYTEST_MONITORING_ENABLED: return - item_markers = {mark.name: mark for mark in item.iter_markers() if mark and mark.name.startswith("monitor_")} + item_markers = { + mark.name: mark + for mark in item.iter_markers() + if mark and mark.name.startswith("monitor_") + } mark_to_del = [] for set_marker in item_markers.keys(): if set_marker not in PYTEST_MONITOR_VALID_MARKERS: - warnings.warn("Nothing known about marker {}. Marker will be dropped.".format(set_marker)) + warnings.warn( + "Nothing known about marker {}. Marker will be dropped.".format( + set_marker + ) + ) mark_to_del.append(set_marker) if set_marker in PYTEST_MONITOR_DEPRECATED_MARKERS: - warnings.warn(f"Marker {set_marker} is deprecated. Consider upgrading your tests") + warnings.warn( + f"Marker {set_marker} is deprecated. Consider upgrading your tests" + ) for marker in mark_to_del: del item_markers[marker] @@ -201,11 +220,20 @@ def wrapped_function(): pyfuncitem.obj(**testargs) except Exception: raise - except BaseException: - raise + except BaseException as e: + # this is a workaround to fix the faulty behavior of the memory profiler + # that only catches Exceptions but should catch BaseExceptions instead + # actually BaseExceptions should be raised here, but without modifications + # of the memory profiler (like proposed in PR + # https://github.com/CFMTech/pytest-monitor/pull/82 ) this problem + # can just be worked around like so (BaseException can only come through + # this way) + return e def prof(): - m = memory_profiler.memory_usage((wrapped_function, ()), max_iterations=1, max_usage=True, retval=True) + m = memory_profiler.memory_usage( + (wrapped_function, ()), max_iterations=1, max_usage=True, retval=True + ) if isinstance(m[1], BaseException): # Do we have any outcome? raise m[1] memuse = m[0][0] if type(m[0]) is list else m[0] @@ -214,9 +242,13 @@ def prof(): if not PYTEST_MONITORING_ENABLED: try: - wrapped_function() + # this is a workaround to fix the faulty behavior of the memory profiler + # that only catches Exceptions but should catch BaseExceptions instead + e = wrapped_function() + if isinstance(e, BaseException): + raise e except BaseException: - raise + raise else: if not pyfuncitem.session.config.option.mtr_disable_gc: gc.collect() @@ -236,12 +268,26 @@ def pytest_sessionstart(session): Instantiate a monitor session to save collected metrics. We yield at the end to let pytest pursue the execution. """ - if session.config.option.mtr_force_component and session.config.option.mtr_component_prefix: - raise pytest.UsageError("Invalid usage: --force-component and --component-prefix are incompatible options!") - if session.config.option.mtr_no_db and not session.config.option.mtr_remote and not session.config.option.mtr_none: - warnings.warn("pytest-monitor: No storage specified but monitoring is requested. Disabling monitoring.") + if ( + session.config.option.mtr_force_component + and session.config.option.mtr_component_prefix + ): + raise pytest.UsageError( + "Invalid usage: --force-component and --component-prefix are incompatible options!" + ) + if ( + session.config.option.mtr_no_db + and not session.config.option.mtr_remote + and not session.config.option.mtr_none + ): + warnings.warn( + "pytest-monitor: No storage specified but monitoring is requested. Disabling monitoring." + ) session.config.option.mtr_none = True - component = session.config.option.mtr_force_component or session.config.option.mtr_component_prefix + component = ( + session.config.option.mtr_force_component + or session.config.option.mtr_component_prefix + ) if session.config.option.mtr_component_prefix: component += ".{user_component}" if not component: @@ -251,13 +297,17 @@ def pytest_sessionstart(session): if (session.config.option.mtr_none or session.config.option.mtr_no_db) else session.config.option.mtr_db_out ) - remote = None if session.config.option.mtr_none else session.config.option.mtr_remote + remote = ( + None if session.config.option.mtr_none else session.config.option.mtr_remote + ) session.pytest_monitor = PyTestMonitorSession( db=db, remote=remote, component=component, scope=session.config.option.mtr_scope ) global PYTEST_MONITORING_ENABLED PYTEST_MONITORING_ENABLED = not session.config.option.mtr_none - session.pytest_monitor.compute_info(session.config.option.mtr_description, session.config.option.mtr_tags) + session.pytest_monitor.compute_info( + session.config.option.mtr_description, session.config.option.mtr_tags + ) yield @@ -298,7 +348,9 @@ def _prf_tracer(request): ptimes_a = request.session.pytest_monitor.process.cpu_times() yield ptimes_b = request.session.pytest_monitor.process.cpu_times() - if not request.node.monitor_skip_test and getattr(request.node, "monitor_results", False): + if not request.node.monitor_skip_test and getattr( + request.node, "monitor_results", False + ): item_name = request.node.originalname or request.node.name item_loc = getattr(request.node, PYTEST_MONITOR_ITEM_LOC_MEMBER)[0] request.session.pytest_monitor.add_test_info(