slurp unnamed test watches properly #5969
Merged
Conversation
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
Contributor
|
Was there no way to intentionally trigger it with a transcript? |
aryairani
approved these changes
Oct 28, 2025
Contributor
|
Or, I guess I was confused about testing strategy between the comment about testing manually and the one about updating builtins.md |
aryairani
approved these changes
Oct 29, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Overview
This PR fixes a small bug in slurp related to unnamed test watches. Previously, we assumed that, when trying to resolve a term by
Nameto its type, itsName.toVar namewould either be found in the latest typechecked unison file, else it'd be in the codebase.Now, between those two, we check whether it exists as an unnamed test watch in the latest typechecked unison file (whose generated
Name, i.e.test.<guid>, matches the name we're looking for.I also amended
builtins.mdto include a test that looks like it was accidentally omitted.Test coverage
I tested this change manually by merging into #5962 where the bug was observed. I don't entirely have an explanation for why that branch was hitting this error but trunk wasn't, as the changes to the load logic are superficial.