-
Notifications
You must be signed in to change notification settings - Fork 16
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
Conversation
There was a problem hiding this 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/test/test_env.py
Outdated
@@ -11,3 +12,17 @@ def test_disable_temporarily(): | |||
except RuntimeError: | |||
... | |||
assert env.enables("requests") | |||
|
|||
def test_determine_log_file(monkeypatch): | |||
env = Env({"_APPMAP": "true"}) |
There was a problem hiding this comment.
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.
_appmap/test/test_env.py
Outdated
|
||
def test_determine_log_file(monkeypatch): | ||
env = Env({"_APPMAP": "true"}) | ||
|
There was a problem hiding this comment.
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?
be47434
to
088dfa7
Compare
088dfa7
to
c179b86
Compare
There was a problem hiding this 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
Fixes #339.
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.appmap.log
file to Docker and adds a test to the smoke test suite for running with a non-modifiableappmap.log
.