You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
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.
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.
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 :)
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.
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.
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
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.
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 (exceptDebug.toTextandValue.value, which is enables). If the check fails, a fairly opaque message is printed out. You can end up in this situation becausetestruns things this way, buttest>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
testto 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:But won't anymore, because
printLineisn't actually called with enough arguments.I also made the following adjustments to the sandboxed runtime
Debug.tracea 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.Debug.toTextin the sandboxed runtime. This was enabled by tests previously.Value.valuewas already allowed in the sandboxed runtime.Addresses #4685. I thought there was another issue asking for either
traceortoTextin docs, which this would also allow (with no-op behavior for the former), but I can't seem to find it.