Skip to content

Change the test command to use the sandboxed runtime instead of evalPureUnison#6169

Merged
aryairani merged 4 commits intotrunkfrom
topic/test-sandbox
Feb 11, 2026
Merged

Change the test command to use the sandboxed runtime instead of evalPureUnison#6169
aryairani merged 4 commits intotrunkfrom
topic/test-sandbox

Conversation

@dolio
Copy link
Contributor

@dolio dolio commented Feb 9, 2026

This PR reworks a few things to improve the behavior of tests. Currently, tests run via evalPureUnison, which surrounds the term with a sandboxing check, looking for any reference to a disallowed term (except Debug.toText and Value.value, which is enables). If the check fails, a fairly opaque message is printed out. You can end up in this situation because test runs things this way, but test> watches are relatively unrestricted. So, you can have a test that appears to be fine during creation, but will fail due to sandboxing later.

This instead changes test to use the sandboxed runtime, with no up front check. This has sensitive operations replaced to throw exceptions, so it's still not possible to use them. But, the error messages will be more targeted (mentioning the disallowed operation), and it won't fail merely because a disallowed operation is referred to, only if it's actually run. E.G. the following would previously fail as a test:

foo =
  unused = printLine
  [Ok "done"]

But won't anymore, because printLine isn't actually called with enough arguments.

I also made the following adjustments to the sandboxed runtime

  • Made Debug.trace a no-op when running in sandboxed mode, instead of throwing an exception. This means that if you forget a trace statement in a test, it just won't do anything, instead of causing a failing test.
  • Allowed Debug.toText in the sandboxed runtime. This was enabled by tests previously.

Value.value was already allowed in the sandboxed runtime.

Addresses #4685. I thought there was another issue asking for either trace or toText in docs, which this would also allow (with no-op behavior for the former), but I can't seem to find it.

Copy link
Member

@pchiusano pchiusano left a comment

Choose a reason for hiding this comment

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

nice!

@aryairani
Copy link
Contributor

Thanks for the quick turnaround @dolio; could you check on that failing transcript and decide what the expected behavior should be; adding :error or changing the tests if appropriate.

cc @ceedubs

@ceedubs ceedubs requested a review from SystemFw February 9, 2026 21:47
@ceedubs
Copy link
Contributor

ceedubs commented Feb 9, 2026

Nice thank you @dolio! Adding @SystemFw as a reviewer since he recently said that he wishes the test sandbox worked this way.

@SystemFw
Copy link
Contributor

SystemFw commented Feb 9, 2026

Nice! @ceedubs this actually means we can have a single list of tests in cloud.internal.test again I think. I'll do that work once this is merged if you think a single list is preferable :)

@dolio
Copy link
Contributor Author

dolio commented Feb 9, 2026

Tweaked the transcript to use a printLine implementation, because Debug.trace no longer generates an error. Let me know if you think it's too out-there.

@ceedubs
Copy link
Contributor

ceedubs commented Feb 10, 2026

Nice! @ceedubs this actually means we can have a single list of tests in cloud.internal.test again I think. I'll do that work once this is merged if you think a single list is preferable :)

I think that to make CI happy we'd need to wait until this actually makes it into a release and not just merged to trunk. Not sure what that timeline will look like and how it compares to when you want to get your changes into /main of the cloud client.

@dolio
Copy link
Contributor Author

dolio commented Feb 10, 2026

Fixes #4904

@aryairani aryairani merged commit 8ce3cd7 into trunk Feb 11, 2026
31 checks passed
@aryairani aryairani deleted the topic/test-sandbox branch February 11, 2026 01:26
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.

5 participants