-
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
try to avoid recording tests #354
Conversation
In addition to the path of config file, show the packages property, too.
Add APPMAP_INSTRUMENT_PROPERTIES to control whether properties should be instrumented.
74a63b8
to
e1af996
Compare
e1af996
to
b133145
Compare
_appmap/importer.py
Outdated
if new_fn != fn: | ||
new_fn = wrapt.FunctionWrapper(fn, new_fn) | ||
setattr(new_fn, "_appmap_wrapped", True) | ||
# Set _appmap_wrapped on the FunctionWrapper, not on the wrapped function |
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.
_appmap_instrumented
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 catch, thanks.
@@ -524,6 +525,7 @@ def __init__(self, wrapped, instance, wrapper, enabled=None, | |||
object.__setattr__(self, '_self_binding', binding) | |||
object.__setattr__(self, '_self_parent', parent) | |||
object.__setattr__(self, '_bfws', list()) | |||
object.__setattr__(self, "_appmap_instrumented", False) |
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'm not sure how I feel about modifying a vendored lib. I'm worried it'll be too easy to miss it when we update or if we ever decide to switch to non-vendored.
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.
This isn't the first change I've had to make to wrapt
, so it's already the case that we have to merge from upstream, rather than just overwriting. Those other changes will also keep us from switching to non-vendored....
Base the decision to instrument a property on the property name, rather than trying to use the name of the f{get,set,del} functions. This aligns them with the rest of the class's members, e.g. for exclusion. Also, ensure that those functions only get instrumented once.
When creating the default config, ignore directories with names that match the regex .*test.*. This should avoid instrumenting the majority of test functions. For those that do still get instrumented, have the test framework integration disable the wrapt wrapped on it, thereby disabling recording.
Add ruff to dev dependencies, along with an example config. At some point in the future, we may want to switch to using it, rather than pylint.
The latest update to incremental (which twisted depends on), appears to be broken. Pin to the previous version.
c0197b6
to
7538fa7
Compare
When creating the default config, ignore directories with names that
match the regex .test.. This should avoid instrumenting the majority
of test functions. For those that do still get instrumented, have the
test framework integration disable the wrapt wrapped on it, thereby
disabling recording.
Also, some issues I found with the way the f{set,get,del} functions of properties were handled. And, adds an environment variable to disable instrumenting properties altogether.
Fixes #351