Skip to content
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

Test touchups in preparation for ir migration #3844

Merged
merged 5 commits into from
Jan 15, 2024

Conversation

tybug
Copy link
Member

@tybug tybug commented Jan 15, 2024

Split from #3818. Follow-up to #3835.

Contains the simpler changes I could split out, including some test changes I missed last time. I'll also open another PR shortly (tomorrow, likely) with more substantive changes, but I've intentionally split them so they can be considered independently.

Hopefully these are the last pulls necessary before the main event!

@tybug tybug requested a review from Zac-HD as a code owner January 15, 2024 04:26
@tybug tybug mentioned this pull request Jan 15, 2024
5 tasks
Comment on lines -1781 to -1789
def write(self, string: bytes) -> Optional[bytes]:
"""Write ``string`` to the output buffer."""
self.__assert_not_frozen("write")
string = bytes(string)
if not string:
return None
self.draw_bits(len(string) * 8, forced=int_from_bytes(string))
return self.buffer[-len(string) :]

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removed in favor of forced on all ir nodes.

Comment on lines 300 to +304
shrinker.fixate_shrink_passes(["minimize_individual_blocks"])
assert list(shrinker.shrink_target.buffer) == [1, 1, 0, 1, 0, 0, 1]
# the minimal bitstream here is actually b'\x01\x01\x00\x00\x01\x00\x00\x01',
# but the shrinker can't discover that it can shrink the size down from 64
# to 32...
assert list(shrinker.shrink_target.buffer) == [3, 1, 0, 0, 0, 0, 0, 0, 1, 0, 0, 1]
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one is interesting. The minimal example here is actually b'\x01\x01\x00\x00\x01\x00\x00\x01'. Even if we run the full .shrink() pass, the shrinker fails to discover this. It doesn't recognize that it can shrink the leading \x03\x01 to \x01\x01 (size = 64 and size = 32 after sampling respectively), so it finds the lowest 64 bit integer it can. In this case that happens to be the same minimal falsifying example of result = 32768, just not the minimal bitstream that represents that example.

Since we're about to rebuild the shrinker anyway, I'd argue we accept this as-is for now. I do wish I had time to figure out why we're failing here, but whatever I discover is unlikely to transfer to the rebuilt shrinker, so... /shrug

Comment on lines 1109 to 1117
[
lambda ix: Ellipsis in ix,
lambda ix: Ellipsis not in ix,
lambda ix: np.newaxis in ix,
lambda ix: np.newaxis not in ix,
lambda ix: isinstance(ix, tuple) and Ellipsis in ix,
lambda ix: isinstance(ix, tuple) and Ellipsis not in ix,
lambda ix: isinstance(ix, tuple) and np.newaxis in ix,
lambda ix: isinstance(ix, tuple) and np.newaxis not in ix,
lambda ix: ix is Ellipsis,
lambda ix: ix == np.newaxis,
],
)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this test only passed before by a stroke of fate: it finds the example it needs and bails early, before it hits a case where it tries to iterate over a non-tuple.

This doesn't get hit on master, but it will get hit on #3818, so I'm fixing it early here.

Copy link
Member

@Zac-HD Zac-HD left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great!

@@ -0,0 +1,3 @@
RELEASE_TYPE: patch

This patch refactors some internals, continuing our work on supporting alternative backends (:issue:`3086`). There is no user-visible change.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, we're failing tests because this is exactly identical to an existing changelog entry! Let's push a newline...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

aha, will keep this hack in mind 😄

@Zac-HD Zac-HD enabled auto-merge January 15, 2024 06:16
@Zac-HD Zac-HD merged commit 303fe43 into HypothesisWorks:master Jan 15, 2024
47 checks passed
@tybug tybug deleted the test-touchups branch January 15, 2024 15:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants