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.

Sign up for GitHub

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

Jump to bottom

fix for test watches in update #4413

Merged
merged 9 commits into from
Nov 22, 2023
Merged

fix for test watches in update #4413

merged 9 commits into from
Nov 22, 2023

Conversation

aryairani
Copy link
Contributor

@aryairani aryairani commented Nov 21, 2023 •
edited
Loading

A few changes to fix #4405:

  • Change update to render tests (terms having type [Test.Result]) as test watches when recreating a scratch file.
  • Hide the test type signature when prettyUnisonFile-ing those tests, since it is added automatically by the parser.

Also fixes #4410.

Overview

Changes the round-trip under update of

test> mynamespace.foo.test = if (foo 2) == 2 then [ Ok "passed" ] else [ Fail "wat" ]

from

mynamespace.foo.test : [Result]
mynamespace.foo.test = if foo 2 == 2 then [Ok "passed"] else [Fail "wat"]

or worse,

mynamespace.foo.test : [Result]
mynamespace.foo.test = if foo 2 == 2 then [Ok "passed"] else [Fail "wat"]

test> mynamespace.foo.test = if (foo 2) == 2 then [ Ok "passed" ] else [ Fail "wat" ]

to

test> mynamespace.foo.test = if (foo 2) == 2 then [ Ok "passed" ] else [ Fail "wat" ]

Test coverage

Adds a couple of transcripts.

Loose ends

aryairani and others added 9 commits November 20, 2023 12:36
@aryairani
add [Test.Result] terms into bigUnisonFile as watched
e2878f4
@aryairani
Add two transcripts
5604a48 
a failing transcript `add-test-watch-roundtrip`
and an ok-looking transcript for `update-test-watch-roundtrip`; though noting that the test gains a type signature.
@aryairani
use Typechecker.isEqual instead of ==
3659e40 
 not sure if this is actually better
@aryairani
add test rewrites, and skip the cleanup of rewrites-tmp.u, to help de…
dde858e 
…bugging.

we could alternatively move the creation and cleanup back into the Unison code,
conditional on an environment variable we set to suppress cleanup.
@tstat
render test watches without type annotations
c037bd4
@aryairani
update transcript outputs
0617160
@aryairani
revert changes to rewrites.md
2cdbf5a
@aryairani
more transcripts
c4db4bd
@aryairani
why do the transcripts' white space change?
fabba1b
@aryairani aryairani added the ready-to-merge Apply this to a PR and it will get merged automatically once CI passes and 1 reviewer has approved label Nov 22, 2023
@aryairani aryairani marked this pull request as ready for review November 22, 2023 03:58
@aryairani aryairani requested a review from ceedubs November 22, 2023 04:42
ceedubs
ceedubs reviewed Nov 22, 2023
View reviewed changes
Copy link
Contributor

@ceedubs ceedubs left a comment

Choose a reason for hiding this comment

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

I see at least one place where your transcript reports that there is a current bug. But I see another where it seems to be adding the double type annotation that I thought should be fixed b this PR. So I'm confused.

unison-src/transcripts/add-test-watch-roundtrip.md
@@ -0,0 +1,15 @@
```ucm:hide
.> builtins.mergeio
Copy link
Contributor

Choose a reason for hiding this comment

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

Off topic: why do both builtins.merge and builtins.mergeio exist, and is there a reason to prefer mergeio here?

Copy link
Contributor Author

@aryairani aryairani Nov 22, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Good question. builtins.merge just includes "builtins" (from the point of view of the runtime). So that's primitive types like Int, Nat, primitive terms like Nat.+.

builtins.mergeio, includes all the "builtins" but also all the user-defined types that UCM commands rely on. So Doc, Test.Result, Author, etc. It is defined with actual Unison code embedded in ucm, and parsed/typechecked when you call the command.

IO used to be a user-defined ability, hence the name "builtins.mergeio", and the rest of the Unison-based definitions were lumped in there as well for convenience. Today, IO is a primitive ability, so you actually get it from builtins.merge, and not from builtins.mergeio 😓

Copy link
Contributor Author

@aryairani aryairani Nov 22, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

builtins.merge takes less time to execute, so I prefer that when I can get away with it, since 2-3 seconds per transcript (or whatever it takes to run the parser/typechecker like builtins.mergeio must do) gets multiplied by 100 transcripts or whatever.

Kind of like:
builtins.merge fastest
builtins.mergeio fast-ish
project.create clone @unison/base 😑

(Actually skipping all of them is the fastest.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was mistaken, Test.Result is in the smaller builtins.merge.

unison-src/transcripts/add-test-watch-roundtrip.output.md
.> view foo

foo : [Result]
foo : [Result]
Copy link
Contributor

Choose a reason for hiding this comment

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

This is still showing the incorrect output, isn't it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Confusing; sorry.
Yes, but that transcript doesn't involve update. So it's tracked by loose end #4417.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh I see the "we don't want this to happen" is saying that this is a bug but the transcript is just capturing that it currently happens, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right. I don't think you will actually run into #4417 unless you are manually putting type signatures on your tests as in that transcript though. Let me know if you find otherwise!

ceedubs
ceedubs approved these changes Nov 22, 2023
View reviewed changes
Copy link
Contributor

@ceedubs ceedubs left a comment

Choose a reason for hiding this comment

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

Looks to be an improvement, though tests will probably continue to be not very usable until #4417 is completed.

@mergify mergify bot merged commit bc0788b into trunk Nov 22, 2023
7 checks passed
@mergify mergify bot deleted the fix/update-tests branch November 22, 2023 14:18
@mergify mergify bot removed the ready-to-merge Apply this to a PR and it will get merged automatically once CI passes and 1 reviewer has approved label Nov 22, 2023
@aryairani aryairani mentioned this pull request Nov 27, 2023
Closed
@aryairani aryairani changed the title fix #4405 fix for test watches in update Nov 28, 2023
@aryairani aryairani mentioned this pull request Nov 28, 2023
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ceedubs ceedubs ceedubs approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

test watches don't round-trip under rewrite new update adds double type annotation to tests
3 participants
@aryairani @ceedubs @tstat