-
Notifications
You must be signed in to change notification settings - Fork 578
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
Address #3821 by adding a flag to turn off coverage reporting for observability #3826
Conversation
- Add OBSERVABILITY_COLLECT_COVERAGE exported from internal/observability - Set that variable based on "HYPOTHESIS_EXPERIMENTAL_OBSERVABILITY_NOCOVER"
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.
Looks good to me! We'll also need a hypothesis-python/RELEASE.rst
, I think for a patch release (see other PRs for examples), and to document this environment variable in the observability docs. Then I think it'll be ready to merge!
@@ -19,6 +19,7 @@ | |||
from hypothesis.internal.conjecture.data import ConjectureData, Status | |||
|
|||
TESTCASE_CALLBACKS: List[Callable[[dict], None]] = [] | |||
OBSERVABILITY_COLLECT_COVERAGE = 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.
Let's set this as OBSERVABILITY_COLLECT_COVERAGE = "HYPOTHESIS_EXPERIMENTAL_OBSERVABILITY_NOCOVER" not in os.environ
, put it just above if "HYPOTHESIS_EXPERIMENTAL_OBSERVABILITY" in os.environ:
, and change that condition to if "HYPOTHESIS_EXPERIMENTAL_OBSERVABILITY" in os.environ or OBSERVABILITY_COLLECT_COVERAGE is False:
.
That way setting only HYPOTHESIS_EXPERIMENTAL_OBSERVABILITY_NOCOVER
will enable observability in nocover mode.
The last thing I'd do is emit a warning in nocover mode on Python >= 3.12, because the sys.monitoring
system is expected to have much lower overhead and users might not know that. Maybe worth a sentence in the docs too?
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.
OK, I think I did all of this correctly!
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.
Looks great - thanks Harrison!
I'll run make format
and push shortly, and then we'll get a release 😁
Not sure if this is correct yet, but I'll make a PR to start a discussion. I added a new command line flag that allows users to turn coverage collection off when doing observability logging.