Skip to content

Commit

Permalink
fix: enabling process recording disables others
Browse files Browse the repository at this point in the history
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).
  • Loading branch information
apotterri committed May 23, 2024
1 parent 31dbdaf commit 32495eb
Show file tree
Hide file tree
Showing 5 changed files with 51 additions and 4 deletions.
16 changes: 14 additions & 2 deletions _appmap/env.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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):
Expand Down
2 changes: 1 addition & 1 deletion _appmap/recording.py
Original file line number Diff line number Diff line change
Expand Up @@ -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()

Expand Down
13 changes: 13 additions & 0 deletions _appmap/test/test_django.py
Original file line number Diff line number Diff line change
Expand Up @@ -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):
Expand Down
17 changes: 16 additions & 1 deletion _appmap/test/test_flask.py
Original file line number Diff line number Diff line change
Expand Up @@ -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 (
Expand Down Expand Up @@ -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):
Expand All @@ -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()
7 changes: 7 additions & 0 deletions _appmap/test/test_test_frameworks.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down

0 comments on commit 32495eb

Please sign in to comment.