From 2df0f37474d1cd26bdfdbb45baf4fd2c9c9c982f Mon Sep 17 00:00:00 2001 From: Alan Potter Date: Sun, 12 May 2024 06:24:54 -0400 Subject: [PATCH 1/5] fix: honor APPMAP_RECORD_REQUESTS when testing Make sure `APPMAP_RECORD_REQUESTS=true` will generate request recordings when running tests. Previously, it was ignored, and request recordings were never generated when tests were run. --- _appmap/env.py | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/_appmap/env.py b/_appmap/env.py index 4de46514..899c4c03 100644 --- a/_appmap/env.py +++ b/_appmap/env.py @@ -73,6 +73,9 @@ def __init__(self, env=None, cwd=None): def set(self, name, value): self._env[name] = value + def setdefault(self, name, default_value): + self._env.setdefault(name, default_value) + def get(self, name, default=None): return self._env.get(name, default) @@ -122,7 +125,7 @@ def enables(self, recording_method, default="true"): def disabled(self, recording_method: str): key = _recording_method_key(recording_method) value = self.get(key) - self.set(key, "false") + self.setdefault(key, "false") try: yield finally: From 500fe55f06c536611e3e292b22a8fade62101afe Mon Sep 17 00:00:00 2001 From: Alan Potter Date: Sun, 12 May 2024 07:49:45 -0400 Subject: [PATCH 2/5] fix: combine testing-related env vars Combine APPMAP_RECORD_PYTEST and APPMAP_RECORD_UNITTEST into APPMAP_RECORD_TESTS. --- _appmap/test/test_runner.py | 2 +- _appmap/test/test_test_frameworks.py | 2 +- appmap/command/runner.py | 3 +-- appmap/pytest.py | 2 +- appmap/unittest.py | 2 +- 5 files changed, 5 insertions(+), 6 deletions(-) diff --git a/_appmap/test/test_runner.py b/_appmap/test/test_runner.py index 176c9401..140df2e2 100644 --- a/_appmap/test/test_runner.py +++ b/_appmap/test/test_runner.py @@ -15,7 +15,7 @@ def test_runner_help(script_runner): assert result.stdout.startswith("usage") -@pytest.mark.parametrize("recording_type", ["process", "pytest", "remote", "requests", "unittest"]) +@pytest.mark.parametrize("recording_type", ["process", "remote", "requests", "tests"]) def test_runner_recording_type(script_runner, recording_type): result = script_runner.run(["appmap-python", "--record", recording_type]) assert result.returncode == 0 diff --git a/_appmap/test/test_test_frameworks.py b/_appmap/test/test_test_frameworks.py index 6cd5e2f1..fbdfb9ec 100644 --- a/_appmap/test/test_test_frameworks.py +++ b/_appmap/test/test_test_frameworks.py @@ -39,7 +39,7 @@ def test_with_appmap_false(self, testdir, monkeypatch): assert not testdir.output().exists() def test_disabled(self, testdir, monkeypatch): - monkeypatch.setenv(f"APPMAP_RECORD_{self._test_type.upper()}", "false") + monkeypatch.setenv("APPMAP_RECORD_TESTS", "false") self.run_tests(testdir) assert not testdir.output().exists() diff --git a/appmap/command/runner.py b/appmap/command/runner.py index 2841b467..20bd5614 100644 --- a/appmap/command/runner.py +++ b/appmap/command/runner.py @@ -24,10 +24,9 @@ _RECORDING_TYPES = set( [ "process", - "pytest", "remote", "requests", - "unittest", + "tests", ] ) diff --git a/appmap/pytest.py b/appmap/pytest.py index 14c8d9f9..5638e063 100644 --- a/appmap/pytest.py +++ b/appmap/pytest.py @@ -22,7 +22,7 @@ def __call__(self, wrapped, _, args, kwargs): return wrapped(*args, **kwargs) -if not Env.current.is_appmap_repo and Env.current.enables("pytest"): +if not Env.current.is_appmap_repo and Env.current.enables("tests"): logger.debug("Test recording is enabled (Pytest)") @pytest.hookimpl diff --git a/appmap/unittest.py b/appmap/unittest.py index 6734cf9c..79951d95 100644 --- a/appmap/unittest.py +++ b/appmap/unittest.py @@ -2,7 +2,7 @@ logger = Env.current.getLogger(__name__) -if not Env.current.is_appmap_repo and Env.current.enables("unittest"): +if not Env.current.is_appmap_repo and Env.current.enables("tests"): logger.debug("Test recording is enabled (unittest)") # pylint: disable=unused-import import _appmap.unittest # pyright: ignore # noqa: F401 From aa89150970ed94d4c2819f8bbe1ab84c1e80c705 Mon Sep 17 00:00:00 2001 From: Alan Potter Date: Mon, 13 May 2024 16:55:55 -0400 Subject: [PATCH 3/5] refactor: rename doc to docs Also, fix a typo. --- {doc => docs}/recording-env-vars.md | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) rename {doc => docs}/recording-env-vars.md (81%) diff --git a/doc/recording-env-vars.md b/docs/recording-env-vars.md similarity index 81% rename from doc/recording-env-vars.md rename to docs/recording-env-vars.md index 92c5297c..89ba7df0 100644 --- a/doc/recording-env-vars.md +++ b/docs/recording-env-vars.md @@ -3,11 +3,11 @@ recording types. In each case, ✓ means that the corresponding recording type will be produced, ❌ means that it will not. ## Web Apps -These tables describe how `APPMAP_RECORD_REQUEST` and `APPMAP_RECORD_REMOTE` are +These tables describe how `APPMAP_RECORD_REQUESTS` and `APPMAP_RECORD_REMOTE` are handled when running a web app. "web app, debug on" means a Flask app run as `flask --debug`, a FastAPI app run using `uvicorn --reload` and, a Django app run with `DEBUG = True` in `settings.py`. -| | `APPMAP_RECORD_REQUEST` is unset | `APPMAP_RECORD_REQUEST` == "true" | `APPMAP_RECORD_REQUEST` == "false" | +| | `APPMAP_RECORD_REQUESTS` is unset | `APPMAP_RECORD_REQUESTS` == "true" | `APPMAP_RECORD_REQUESTS` == "false" | | -------------------- | :----------------------------: | :------------------------------: | :-------------------------------: | | "web app, debug on" | ✓ | ✓ | ❌ | | "web app, debug off" | ✓ | ✓ | ❌ | @@ -21,13 +21,13 @@ a FastAPI app run using `uvicorn --reload` and, a Django app run with `DEBUG = T ## Testing This table shows how `APPMAP_RECORD_PYTEST`, `APPMAP_RECORD_UNITTEST`, and -`APPMAP_RECORD_REQUEST` are handled when running tests in. Note that in v2, in +`APPMAP_RECORD_REQUESTS` are handled when running tests in. Note that in v2, in v2, `APPMAP_RECORD_PYTEST` and `APPMAP_RECORD_UNITTEST` will be replaced with -APPMAP_RECORD_TEST. +`APPMAP_RECORD_TESTS`. -| | `APPMAP_RECORD_PYTEST` is unset | `APPMAP_RECORD_PYTEST` == "true" | `APPMAP_RECORD_PYTEST` == "false" | `APPMAP_RECORD_REQUEST` is unset | `APPMAP_RECORD_REQUEST` == "true" | `APPMAP_RECORD_REQUEST` == "false" | +| | `APPMAP_RECORD_PYTEST` is unset | `APPMAP_RECORD_PYTEST` == "true" | `APPMAP_RECORD_PYTEST` == "false" | `APPMAP_RECORD_REQUESTS` is unset | `APPMAP_RECORD_REQUESTS` == "true" | `APPMAP_RECORD_REQUESTS` == "false" | | ------ | :---------------------------: | :-----------------------------: | :------------------------------: | :----------------------------: | :-----------------------------: | :------------------------------: | -| pytest | ✓ | ✓ | ❌ | ✓in v1, ❌ in v2 | ✓ | ❌ | +| pytest | ✓ | ✓ | ❌ | ❌ | ignored in v1, ✓ in v2 | ❌ | From 57b3910a48cea8582612772d79abacc53b5b73d5 Mon Sep 17 00:00:00 2001 From: Alan Potter Date: Mon, 13 May 2024 16:58:14 -0400 Subject: [PATCH 4/5] feat: disable record by default Recording is no longer enabled by default. It is now necessary to run scripts with appmap-python, or explicitly set APPMAP=true BREAKING CHANGE: disable record by default --- README.md | 6 ++- _appmap/env.py | 27 +---------- _appmap/recording.py | 3 -- _appmap/test/test_configuration.py | 6 --- _appmap/unittest.py | 3 -- _appmap/web_framework.py | 2 - appmap/__init__.py | 74 +++++++++++++++--------------- appmap/pytest.py | 4 -- tox.ini | 10 ++-- 9 files changed, 47 insertions(+), 88 deletions(-) diff --git a/README.md b/README.md index 5d16a3ab..cf878433 100644 --- a/README.md +++ b/README.md @@ -87,9 +87,13 @@ reenabled as soon as possible.] ### pytest Note that you must install the dependencies contained in -[requirements-test.txt](requirements-test.txt) before running tests. See the explanation in +[requirements-dev.txt](requirements-dev.txt) before running tests. See the explanation in [pyproject.toml](pyproject.toml) for details. +Additionally, the tests currently require that you set `APPMAP=true`. You can +either run `pytest` with `appmap-python` (see [tox.ini](tox.ini)), or you can explicitly +set the environment variable. + [pytest](https://docs.pytest.org/en/stable/) for testing: ``` diff --git a/_appmap/env.py b/_appmap/env.py index 899c4c03..645412f2 100644 --- a/_appmap/env.py +++ b/_appmap/env.py @@ -3,7 +3,6 @@ import logging import logging.config import os -import warnings from contextlib import contextmanager from os import environ from pathlib import Path @@ -13,20 +12,6 @@ from . import trace_logger -_ENABLED_BY_DEFAULT_MSG = """ - -The APPMAP environment variable is unset. Your code will be -instrumented and recorded according to the configuration in appmap.yml. - -Starting with version 2, this behavior will change: when APPMAP is -unset, no code will be instrumented. You will need to use the -appmap-python script to run your application, or explicitly set -APPMAP. - -Visit https://appmap.io/docs/reference/appmap-python.html#appmap-python-script for more -details. -""" - _cwd = Path.cwd() _bootenv = environ.copy() @@ -36,9 +21,8 @@ def _recording_method_key(recording_method): class Env(metaclass=SingletonMeta): - def __init__(self, env=None, cwd=None): - warnings.filterwarnings("once", _ENABLED_BY_DEFAULT_MSG) + def __init__(self, env=None, cwd=None): # root_dir and root_dir_len are going to be used when # instrumenting every function, so preprocess them as # much as possible. @@ -54,7 +38,6 @@ def __init__(self, env=None, cwd=None): self._configure_logging() enabled = self._env.get("_APPMAP", None) - self._enabled_by_default = enabled is None self._enabled = enabled is None or enabled.lower() != "false" self._root_dir = str(self._cwd) + "/" @@ -98,14 +81,6 @@ def output_dir(self): def output_dir(self, value): self._output_dir = value - @property - def enabled_by_default(self): - return self._enabled_by_default - - def warn_enabled_by_default(self): - if self._enabled_by_default: - warnings.warn(_ENABLED_BY_DEFAULT_MSG, category=DeprecationWarning, stacklevel=2) - @property def enabled(self): return self._enabled diff --git a/_appmap/recording.py b/_appmap/recording.py index 465a5e1b..8aed253e 100644 --- a/_appmap/recording.py +++ b/_appmap/recording.py @@ -44,7 +44,6 @@ def is_running(self): return Recorder.get_enabled() def __enter__(self): - Env.current.warn_enabled_by_default() self.start() def __exit__(self, exc_type, exc_value, tb): @@ -81,8 +80,6 @@ def write_appmap( def initialize(): if Env.current.enables("process", "false"): - Env.current.warn_enabled_by_default() - r = Recording() r.start() diff --git a/_appmap/test/test_configuration.py b/_appmap/test/test_configuration.py index 71b26a8a..ecd73cb4 100644 --- a/_appmap/test/test_configuration.py +++ b/_appmap/test/test_configuration.py @@ -16,10 +16,6 @@ from _appmap.importer import Filterable, NullFilter -def test_enabled_by_default(): - assert appmap.enabled() - - @pytest.mark.appmap_enabled def test_can_be_configured(): """ @@ -45,8 +41,6 @@ def test_reports_invalid(): @pytest.mark.appmap_enabled(config="appmap-broken.yml") def test_is_disabled_when_unset(): """Test that recording is disabled when APPMAP is unset but the config is broken""" - assert Env.current.get("_APPMAP", None) is None - assert not appmap.enabled() diff --git a/_appmap/unittest.py b/_appmap/unittest.py index 18fcfed6..7641a86b 100644 --- a/_appmap/unittest.py +++ b/_appmap/unittest.py @@ -3,7 +3,6 @@ from contextlib import contextmanager from _appmap import noappmap, testing_framework, wrapt -from _appmap.env import Env from _appmap.utils import get_function_location _session = testing_framework.session("unittest", "tests") @@ -43,7 +42,6 @@ def _args(test_case, *_, isTest=False, **__): with _session.record( test_case.__class__, method_name, location=location ) as metadata: - Env.current.warn_enabled_by_default() if metadata: with wrapped( *args, **kwargs @@ -69,7 +67,6 @@ def callTestMethod(wrapped, test_case, args, kwargs): method_name = test_case.id().split(".")[-1] location = _get_test_location(test_case.__class__, method_name) with _session.record(test_case.__class__, method_name, location=location) as metadata: - Env.current.warn_enabled_by_default() if metadata: with testing_framework.collect_result_metadata(metadata): wrapped(*args, **kwargs) diff --git a/_appmap/web_framework.py b/_appmap/web_framework.py index 9034f474..aebf08a7 100644 --- a/_appmap/web_framework.py +++ b/_appmap/web_framework.py @@ -250,8 +250,6 @@ def remote_enabled(self): """Return True if the AppMap middleware has enabled remote recording, False otherwise.""" def run(self): - Env.current.warn_enabled_by_default() - if not self.middleware_present(): return self.insert_middleware() diff --git a/appmap/__init__.py b/appmap/__init__.py index 7c58b247..5d271a40 100644 --- a/appmap/__init__.py +++ b/appmap/__init__.py @@ -9,40 +9,40 @@ if _enabled is None or _enabled.upper() == "TRUE": if _enabled is not None: # Use setdefault so tests can manage _APPMAP as necessary - os.environ["_APPMAP"] = _enabled - from _appmap import generation # noqa: F401 - from _appmap.env import Env # noqa: F401 - from _appmap.importer import instrument_module # noqa: F401 - from _appmap.labels import labels # noqa: F401 - from _appmap.noappmap import decorator as noappmap # noqa: F401 - from _appmap.recording import Recording # noqa: F401 - - try: - from . import django # noqa: F401 - except ImportError: - pass - - try: - from . import flask # noqa: F401 - except ImportError: - pass - - try: - from . import fastapi # noqa: F401 - except ImportError: - pass - - try: - from . import uvicorn # noqa: F401 - except ImportError: - pass - - # Note: pytest integration is configured as a pytest plugin, so it doesn't - # need to be imported here - - # unittest is part of the standard library, so it should always be - # importable (and therefore doesn't need to be in a try .. except block) - from . import unittest # noqa: F401 - - def enabled(): - return Env.current.enabled + os.environ.setdefault("_APPMAP", _enabled) + from _appmap import generation # noqa: F401 + from _appmap.env import Env # noqa: F401 + from _appmap.importer import instrument_module # noqa: F401 + from _appmap.labels import labels # noqa: F401 + from _appmap.noappmap import decorator as noappmap # noqa: F401 + from _appmap.recording import Recording # noqa: F401 + + try: + from . import django # noqa: F401 + except ImportError: + pass + + try: + from . import flask # noqa: F401 + except ImportError: + pass + + try: + from . import fastapi # noqa: F401 + except ImportError: + pass + + try: + from . import uvicorn # noqa: F401 + except ImportError: + pass + + # Note: pytest integration is configured as a pytest plugin, so it doesn't + # need to be imported here + + # unittest is part of the standard library, so it should always be + # importable (and therefore doesn't need to be in a try .. except block) + from . import unittest # noqa: F401 + + def enabled(): + return Env.current.enabled diff --git a/appmap/pytest.py b/appmap/pytest.py index 5638e063..8c555b52 100644 --- a/appmap/pytest.py +++ b/appmap/pytest.py @@ -25,10 +25,6 @@ def __call__(self, wrapped, _, args, kwargs): if not Env.current.is_appmap_repo and Env.current.enables("tests"): logger.debug("Test recording is enabled (Pytest)") - @pytest.hookimpl - def pytest_configure(config): # pylint: disable=unused-argument - Env.current.warn_enabled_by_default() - @pytest.hookimpl def pytest_sessionstart(session): session.appmap = testing_framework.session( diff --git a/tox.ini b/tox.ini index 3fe7c822..ea4ec756 100644 --- a/tox.ini +++ b/tox.ini @@ -24,14 +24,12 @@ deps= commands = poetry install -v - web: poetry run {posargs:pytest} - django3: poetry run pytest _appmap/test/test_django.py - flask2: poetry run pytest _appmap/test/test_flask.py - sqlalchemy1: poetry run pytest _appmap/test/test_sqlalchemy.py + web: poetry run appmap-python {posargs:pytest} + django3: poetry run appmap-python pytest _appmap/test/test_django.py + flask2: poetry run appmap-python pytest _appmap/test/test_flask.py + sqlalchemy1: poetry run appmap-python pytest _appmap/test/test_sqlalchemy.py [testenv:lint] -setenv = - APPMAP=false skip_install = True deps = poetry From 74b2ee15bfc380ee44ef74905e880541028f4c3b Mon Sep 17 00:00:00 2001 From: Alan Potter Date: Mon, 20 May 2024 17:32:11 -0400 Subject: [PATCH 5/5] fix: enabling process recording disables others It doesn't make sense (and doesn't work) to try to capture other recording types when the user says they want a process recording (by setting APPMAP_RECORD_PROCESS=true). --- _appmap/env.py | 16 ++++++++++++++-- _appmap/recording.py | 2 +- _appmap/test/test_django.py | 13 +++++++++++++ _appmap/test/test_flask.py | 17 ++++++++++++++++- _appmap/test/test_test_frameworks.py | 7 +++++++ 5 files changed, 51 insertions(+), 4 deletions(-) diff --git a/_appmap/env.py b/_appmap/env.py index 645412f2..8d95aa3e 100644 --- a/_appmap/env.py +++ b/_appmap/env.py @@ -21,6 +21,7 @@ def _recording_method_key(recording_method): class Env(metaclass=SingletonMeta): + RECORD_PROCESS_DEFAULT = "false" def __init__(self, env=None, cwd=None): # root_dir and root_dir_len are going to be used when @@ -93,8 +94,19 @@ def enables(self, recording_method, default="true"): if not self.enabled: return False - v = self.get(_recording_method_key(recording_method), default).lower() - return v != "false" + process_enabled = self._enables("process", self.RECORD_PROCESS_DEFAULT) + if recording_method == "process": + return process_enabled + + # If process recording is enabled, others should be disabled + if process_enabled: + return False + + # Otherwise, check the environment variable + return self._enables(recording_method, default) + + def _enables(self, recording_method, default): + return self.get(_recording_method_key(recording_method), default).lower() != "false" @contextmanager def disabled(self, recording_method: str): diff --git a/_appmap/recording.py b/_appmap/recording.py index 8aed253e..ea05b0d7 100644 --- a/_appmap/recording.py +++ b/_appmap/recording.py @@ -79,7 +79,7 @@ def write_appmap( def initialize(): - if Env.current.enables("process", "false"): + if Env.current.enables("process", Env.RECORD_PROCESS_DEFAULT): r = Recording() r.start() diff --git a/_appmap/test/test_django.py b/_appmap/test/test_django.py index d26bb73a..1e9ee1dc 100644 --- a/_appmap/test/test_django.py +++ b/_appmap/test/test_django.py @@ -216,6 +216,19 @@ def test_disabled(self, pytester, monkeypatch): result.assert_outcomes(passed=1, failed=0, errors=0) assert not (pytester.path / "tmp").exists() + def test_disabled_for_process(self, pytester, monkeypatch): + monkeypatch.setenv("APPMAP_RECORD_PROCESS", "true") + + result = pytester.runpytest("-svv") + + # There are two tests for remote recording. They should both fail, + # because process recording should disable remote recording. + result.assert_outcomes(passed=2, failed=2, errors=0) + + assert (pytester.path / "tmp" / "appmap" / "process").exists() + assert not (pytester.path / "tmp" / "appmap" / "requests").exists() + assert not (pytester.path / "tmp" / "appmap" / "pytest").exists() + @pytest.fixture(name="server") def django_server(xprocess, server_base): diff --git a/_appmap/test/test_flask.py b/_appmap/test/test_flask.py index 9bd1eb5a..9bb20c50 100644 --- a/_appmap/test/test_flask.py +++ b/_appmap/test/test_flask.py @@ -8,10 +8,10 @@ import flask import pytest -from appmap.flask import AppmapFlask from _appmap.env import Env from _appmap.metadata import Metadata +from appmap.flask import AppmapFlask from ..test.helpers import DictIncluding from .web_framework import ( @@ -144,7 +144,11 @@ def test_enabled(self, pytester): appmap_file = ( pytester.path / "tmp" / "appmap" / "pytest" / "test_request.appmap.json" ) + + # No request recordings should have been created assert not os.path.exists(pytester.path / "tmp" / "appmap" / "requests") + + # but there should be a test recording assert appmap_file.exists() def test_disabled(self, pytester, monkeypatch): @@ -154,3 +158,14 @@ def test_disabled(self, pytester, monkeypatch): result.assert_outcomes(passed=1, failed=0, errors=0) assert not (pytester.path / "tmp" / "appmap").exists() + + def test_disabled_for_process(self, pytester, monkeypatch): + monkeypatch.setenv("APPMAP_RECORD_PROCESS", "true") + + result = pytester.runpytest("-svv") + + result.assert_outcomes(passed=1, failed=0, errors=0) + + assert (pytester.path / "tmp" / "appmap" / "process").exists() + assert not (pytester.path / "tmp" / "appmap" / "requests").exists() + assert not (pytester.path / "tmp" / "appmap" / "pytest").exists() diff --git a/_appmap/test/test_test_frameworks.py b/_appmap/test/test_test_frameworks.py index fbdfb9ec..e5093585 100644 --- a/_appmap/test/test_test_frameworks.py +++ b/_appmap/test/test_test_frameworks.py @@ -44,6 +44,13 @@ def test_disabled(self, testdir, monkeypatch): self.run_tests(testdir) assert not testdir.output().exists() + def test_disabled_for_process(self, testdir, monkeypatch): + monkeypatch.setenv("APPMAP_RECORD_PROCESS", "true") + + self.run_tests(testdir) + assert (testdir.path / "tmp" / "appmap" / "process").exists() + assert not testdir.output().exists() + class TestUnittestRunner(_TestTestRunner): @classmethod