try to avoid recording tests#354
Merged
Merged
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.
apotterri
force-pushed
the
no-test-cases_20240715
branch
from
74a63b8 to
e1af996
Compare
apotterri
force-pushed
the
no-test-cases_20240715
branch
from
e1af996 to
b133145
Compare
dividedmind
approved these changes
Jul 26, 2024
| 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 |
Contributor
Author
There was a problem hiding this comment.
Good catch, thanks.
| object.__setattr__(self, '_self_binding', binding) | ||
| object.__setattr__(self, '_self_parent', parent) | ||
| object.__setattr__(self, '_bfws', list()) | ||
| object.__setattr__(self, "_appmap_instrumented", False) |
Contributor
There was a problem hiding this comment.
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.
Contributor
Author
There was a problem hiding this comment.
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.
apotterri
force-pushed
the
no-test-cases_20240715
branch
from
c0197b6 to
7538fa7
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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