-
Notifications
You must be signed in to change notification settings - Fork 202
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
Clean up daml script tests #16570
Clean up daml script tests #16570
Conversation
@@ -22,7 +22,8 @@ final class AuthIT | |||
with Matchers | |||
with SuiteResourceManagementAroundAll { | |||
override def darFile = new File(rlocation("daml-script/test/script-test.dar")) | |||
val (dar, envIface) = readDar(darFile) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
envIface
was never used.
val dar = DarDecoder.assertReadArchiveFromFile(file) | ||
val pkgs = PureCompiledPackages.assertBuild(dar.all.toMap, Runner.compilerConfig) | ||
CompiledDar(dar.main._1, pkgs) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We compiled the package just when we read it and do not do it each time Runner.run
is called
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO, it seems weird that the runner handles input conversion at all, surely that should be the job of whatever is passing in an input?
def tuple(a: SValue, b: SValue) = | ||
SValue.SRecord( | ||
id = DA.Types.Tuple2, | ||
fields = ImmArray(Ref.Name.assertFromString("_1"), Ref.Name.assertFromString("_2")), | ||
values = ArrayList(a, b), | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if theres anywhere else we do this - seems generic enough that it could be outside of a specific test.
I thinking perhaps specifically some of the question answers for ScriptRunner, do any of those return a tuple that we manually create?
def tuple(a: SValue, b: SValue) = | ||
SRecord( | ||
id = DA.Types.Tuple2, | ||
fields = ImmArray(Name.assertFromString("_1"), Name.assertFromString("_2")), | ||
fields = ImmArray(Ref.Name.assertFromString("_1"), Ref.Name.assertFromString("_2")), | ||
values = ArrayList(a, b), | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AbstractScriptTest.tuple?
I noted you question about the converter in the runner and the tuple inside answers. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
I could also maybe tackle these, depending on complexity, but I think I've covered enough scala to do this |
In this PR:
ValueTranslator
instead)EnvironmentSignature
which was never used.