Skip to content
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

fix: appmap breaks vscode python extension starting a REPL #340

Merged
merged 1 commit into from
Jun 20, 2024

Conversation

zermelo-wisen
Copy link
Collaborator

@zermelo-wisen zermelo-wisen commented Jun 17, 2024

Fixes #339.

  • Tolerates the inability to create the appmap.log file for logging, since a file system read-only error can occur in the [VSCode Python Extension, Virtual Environment, REPL, Shift Enter] scenario when attempting to create the log file.
  • Mounts a read-only appmap.log file to Docker and adds a test to the smoke test suite for running with a non-modifiable appmap.log.

Copy link
Collaborator

@apotterri apotterri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good start, a bit more to do.

_appmap/env.py Show resolved Hide resolved
_appmap/env.py Show resolved Hide resolved
@@ -11,3 +12,17 @@ def test_disable_temporarily():
except RuntimeError:
...
assert env.enables("requests")

def test_determine_log_file(monkeypatch):
env = Env({"_APPMAP": "true"})
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you check to see if APPMAP was really "true" when you were investigating #339? I don't think it should have been.


def test_determine_log_file(monkeypatch):
env = Env({"_APPMAP": "true"})

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's important to add an end-to-end test that actually shows that the behavior described in #339 is fixed. It would be ideal to add this to the smoketest, but that's going to be complicated by the fact that processes in docker run as root.

Can you give some thought to how we might implement such a test, please?

@zermelo-wisen zermelo-wisen force-pushed the fix/appmap-breaks-python-extension-repl branch 19 times, most recently from be47434 to 088dfa7 Compare June 20, 2024 09:24
@zermelo-wisen zermelo-wisen force-pushed the fix/appmap-breaks-python-extension-repl branch from 088dfa7 to c179b86 Compare June 20, 2024 09:32
Copy link
Collaborator

@apotterri apotterri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Needs one more minor change, then this will be good to go.

The smoke test is starting to get pretty complicated. At some point, we could maybe simplify it by using something like bats

_appmap/env.py Show resolved Hide resolved
@apotterri apotterri merged commit 8560bff into master Jun 20, 2024
2 checks passed
@apotterri apotterri deleted the fix/appmap-breaks-python-extension-repl branch June 20, 2024 13:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Installing appmap breaks Microsoft's VS Code python extension support for starting a REPL
2 participants