Unify semantic test between sandbox IT and Ledger API Test Tool #1171
Conversation
This file contains 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
gabor-aranyossy
suggested changes
May 16, 2019
...g-utils/src/main/scala/com/digitalasset/ledger/api/testing/utils/AkkaBeforeAndAfterAll.scala
Outdated
Show resolved
Hide resolved
...pi-integration-tests/src/test/lib/scala/com/digitalasset/platform/PlatformApplications.scala
Outdated
Show resolved
Hide resolved
...egration-tests/src/test/lib/scala/com/digitalasset/platform/apitesting/LedgerFactories.scala
Show resolved
Hide resolved
...emanticsuite/scala/com/digitalasset/platform/semantictest/SandboxSemanticTestsLfRunner.scala
Outdated
Show resolved
Hide resolved
ledger/ledger-api-test-tool/src/main/scala/com/daml/ledger/api/testtool/LedgerApiTestTool.scala
Outdated
Show resolved
Hide resolved
ledger/ledger-api-test-tool/src/main/scala/com/daml/ledger/api/testtool/LedgerApiTestTool.scala
Outdated
Show resolved
Hide resolved
gleber-da
force-pushed
the
ledger-api-it-remote-fixture
branch
4 times, most recently
from
f3525bf to
d4ace1c
Compare
gabor-aranyossy
approved these changes
May 20, 2019
...pi-integration-tests/src/test/lib/scala/com/digitalasset/platform/PlatformApplications.scala
Outdated
Show resolved
Hide resolved
| @@ -56,6 +56,19 @@ object LedgerFactories { | |||
|
|
|||
| } | |||
|
|
|||
| def createRemoteApiProxyResource(config: PlatformApplications.Config)( | |||
| implicit esf: ExecutionSequencerFactory): Resource[LedgerContext.SingleChannelContext] = { | |||
| require(config.remoteApiEndpoint.isDefined, "config.remoteApiEndpoint has to be set") | |||
There was a problem hiding this comment.
you could assert on the host and port too
There was a problem hiding this comment.
Why? They are not Option[*], hence should have meaningful values. In worst case we'd have 0 as port and empty string as host, which will just make it fail at runtime.
There was a problem hiding this comment.
host can be empty and port can be negative. It's nit-picking, I just thought I mention it.
...ion-tests/src/test/lib/scala/com/digitalasset/platform/apitesting/RemoteServerResource.scala
Show resolved
Hide resolved
...ion-tests/src/test/lib/scala/com/digitalasset/platform/apitesting/RemoteServerResource.scala
Outdated
Show resolved
Hide resolved
gerolf-da
reviewed
May 20, 2019
...on-tests/src/test/lib/scala/com/digitalasset/platform/apitesting/SandboxServerResource.scala
Outdated
Show resolved
Hide resolved
...ion-tests/src/test/lib/scala/com/digitalasset/platform/apitesting/RemoteServerResource.scala
Outdated
Show resolved
Hide resolved
...on-tests/src/test/lib/scala/com/digitalasset/platform/apitesting/SandboxServerResource.scala
Outdated
Show resolved
Hide resolved
gleber-da
force-pushed
the
ledger-api-it-remote-fixture
branch
from
d4ace1c to
1a19b7b
Compare
gleber-da
force-pushed
the
ledger-api-it-remote-fixture
branch
5 times, most recently
from
284eb40 to
5f1d443
Compare
gleber-da
force-pushed
the
ledger-api-it-remote-fixture
branch
from
5f1d443 to
18fdc26
Compare
gabor-aranyossy
approved these changes
May 27, 2019
|
|
||
| private def repeatChar(char: Char, n: Int) = List.fill(n)(char).mkString | ||
|
|
||
| val depth: AtomicInteger = new AtomicInteger(0) |
There was a problem hiding this comment.
the Atomic* semantics does not seem to be required here. Does a Reporter need to be thread-safe?
There was a problem hiding this comment.
I assumed it had to, but it does not have to. No other implementation I could find was using concurrency synchronization.
There was a problem hiding this comment.
in that case I'd just use a var to avoid confusion
| final val ansiYellow = "\u001b[33m" | ||
| final val ansiRed = "\u001b[31m" | ||
|
|
||
| private def repeatChar(char: Char, n: Int) = List.fill(n)(char).mkString |
There was a problem hiding this comment.
nitpick: this can be really inefficient. Consider creating an array and a String from it via: String.valueOf
There was a problem hiding this comment.
I've switched to StringLike.* which is efficient: https://github.com/scala/scala/blob/v2.12.8/src/library/scala/collection/immutable/StringLike.scala#L72
This is in preparation for using Sandbox IT suite as part of the Ledger API Test Tool.
This is no longer necessary for the tool and it does not scale with the types of tests in the suite.
This makes sure that a server getting stuck will get detected by a test, instead of ignoring it and potentially allowing the server to linger.
It manges parties and command identifier to include a unique (random) suffix in all ledger-commited identifiers. This allows the test to run against a Ledger API without reseting it.
…uite. This reuses the scenario runner test code from the IT suite, instead of reimplementing it. This should be a no-op (except for tests reports formatting).
This makrs Akka threads to be daemons, hence forcing them to be closed at the end of Ledger Api Test Tool.
This quites Ledger API Test Tool output.
gleber-da
force-pushed
the
ledger-api-it-remote-fixture
branch
from
18fdc26 to
802b275
Compare
This addresses two needs: - avoid using buggy scalatest test reporter; - pretty-prints test results prettier.
gleber-da
force-pushed
the
ledger-api-it-remote-fixture
branch
from
802b275 to
0375ea9
Compare
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.
See individual commit messages.
Prepares for #146
Pull Request Checklist
NOTE: CI is not automatically run on non-members pull-requests for security
reasons. The reviewer will have to comment with
/AzurePipelines runtotrigger the build.