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
Add docstrings and comments for Example and Block #1601
Conversation
3729279
to
7fb0462
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.
This looks good to me, and very helpful - but for the same reason I probably shouldn't approve it!
""" | ||
|
||
# Depth of this example in the example tree, which is equal to its number | ||
# of ancestor examples. |
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.
So the first example drawn would have depth 0?
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.
The root example that spans the entire test run has a depth of 0.
Anything that actually gets drawn will end up having a depth of at least 1. I believe the arguments to @given
end up being packed into a tuple strategy, so the first "user-facing" draw should have a depth of at least 2, though I haven't confirmed this.
(Explicitly listing the depth of the root example seems more useful than what I've currently written here.)
7fb0462
to
4443350
Compare
I've pushed a new comment for I've also clarified that the "example structure" refers to the structure of draws within a single test run. (Unfortunately the word "example" gets used with a few different meanings, in different parts of the codebase.) |
I know. :-( I'd like to fix this at some point. |
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.
Thanks for doing this! Generally looks good to me, have added a bunch of nitpicky comments though.
within a single test run. | ||
|
||
An example usually corresponds to a single draw from a strategy, and | ||
includes that strategy's nested draws as children. There are also other |
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.
FWIW I think "Includes that strategy's nested draws as children" is misleading in that there's nothing going on here other than the usual hierarchical structure.
includes that strategy's nested draws as children. There are also other | ||
kinds of examples, such as leaf examples that correspond to individual | ||
low-level blocks, and a single top-level example that spans the entire | ||
input. |
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.
Maybe worth mentioning there are a few other places we use examples for grouping?
""" | ||
|
||
# Depth of this example in the example tree. The top-level example has a | ||
# depth of 0. |
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.
nit: I think maybe these comments should be docstrings, though we're not actually using sphinx's autodoc on these files and I'm not sure how docstrings and attrs interact, so it doesn't matter much.
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.
There would be no runtime effect, and Sphinx would only pick them up if (a) it understands attrs and (b) we used it to generate internal docs. So I'm happy with comments!
|
||
# True if this example can be discarded by the shrinker, without affecting | ||
# the value produced by the enclosing example. Typically set when a | ||
# rejection sampler decides to reject a generated value and try again. |
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.
Possibly "if we believe this example can be..."? There are circumstances (where the strategy has side effects) where an example can be marked as discarded but turn out to be essential.
|
||
Block-tracking allows the shrinker to try "low-level" | ||
transformations, such as minimizing the numeric value of individual | ||
blocks, or of pairs of blocks. |
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 the use of "blocks" here is maybe confusing because it's self-referential. Maybe something along the lines of "tracking these allows us to minimize primitive integer values drawn from the byte stream, either individually or in groups" (I'm not wild about this phrasing, but general idea)
index = attr.ib() | ||
|
||
# True if this block's byte values were forced by a write operation, and | ||
# were not read from the input stream. Modifying those byte values in the | ||
# shrinker is unlikely to be useful. |
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.
nit: Not just unlikely but basically never useful unless values earlier in the byte stream are also changed.
Looks like some useful feedback. I should get a chance to go through it tomorrow. |
4443350
to
8b4ae09
Compare
I've pushed some more changes, which I plan to squash after review. |
6741db7
to
4592c95
Compare
4592c95
to
58d209e
Compare
As suggested by #1594 (comment).
Let me know if anything seems unclear or misleading.