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

started centralizing api Test utils #1863

Merged
merged 6 commits into from
Jul 2, 2019
Merged

started centralizing api Test utils #1863

merged 6 commits into from
Jul 2, 2019

Conversation

gabor-aranyossy
Copy link
Contributor

@gabor-aranyossy gabor-aranyossy commented Jun 25, 2019 •
edited
Loading

contributes to #1746

Centralises testing utilities around:

  • transaction filters
  • offsets
  • parties

Note that TransactionServiceIT is the test bed of the changes, the rest of the test will be ported only after that.

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 run to
trigger the build.

gleber-da
gleber-da reviewed Jun 26, 2019
View reviewed changes
...pi-integration-tests/src/test/lib/scala/com/digitalasset/platform/apitesting/TestUtils.scala Outdated
object TestUtils {

private lazy val ParsedPackageId: String =
UniversalArchiveReader().readFile(Config.defaultDarFile).get.main._1 // Test.dar
Copy link
Contributor

Choose a reason for hiding this comment

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

This wont work in the tool, since the test tool passes a dar file at a different than the default location.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, but this is identical to the state what we have now. I wanted to centralise this logic first and foremost. We can think about how to make it dynamic later.

Copy link
Contributor

Choose a reason for hiding this comment

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

This will break Ledger API Test Tool, which is our publicly exposed tool.

gleber-da
gleber-da reviewed Jun 26, 2019
View reviewed changes
...pi-integration-tests/src/test/lib/scala/com/digitalasset/platform/apitesting/TestUtils.scala Outdated
private val MaximumRecordTime =
LedgerEffectiveTime.copy(seconds = LedgerEffectiveTime.seconds + 30L)

private def randomCommandId: String = UUID.randomUUID().toString
Copy link
Contributor

Choose a reason for hiding this comment

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

This will have to be plugged into the machinery I am setting up to control if identifiers are randomized or not. My draft for this is here: https://github.com/digital-asset/daml/pull/1735/files#diff-5f0c4de564dbea036385a0cda87b7fd9R5

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, let me know when that lands. Note, that this part of the code is not used yet.

@gabor-aranyossy gabor-aranyossy marked this pull request as ready for review June 26, 2019 14:56
gleber-da
gleber-da reviewed Jun 27, 2019
View reviewed changes
...integration-tests/src/test/lib/scala/com/digitalasset/platform/apitesting/TestCommands.scala Outdated
@@ -25,16 +23,9 @@ import com.digitalasset.platform.PlatformApplications
import com.google.protobuf.timestamp.Timestamp
import scalaz.syntax.tag._

class TestTemplateIds(config: PlatformApplications.Config) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Keep this in interim. Next move it into class TestUtils in a separate PR.

@gabor-aranyossy gabor-aranyossy force-pushed the test-utils branch 3 times, most recently from dc5dab0 to b1db03a Compare July 1, 2019 09:36
@gabor-aranyossy
Copy link
Contributor Author

gabor-aranyossy commented Jul 1, 2019 •
edited
Loading

@gleber-da please take another look. I don't get why I have so much formatting related diffs. (I mean how can we have differently formatted code on master)

UPDATE: looks like a still have some things to fix. Weirdly locally it all compiled.

@stefanobaghino-da stefanobaghino-da mentioned this pull request Jul 1, 2019
Merged
5 tasks
stefanobaghino-da
stefanobaghino-da reviewed Jul 1, 2019
View reviewed changes
...pi-integration-tests/src/test/lib/scala/com/digitalasset/platform/apitesting/TestUtils.scala

}

object LedgerOffsets {
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 just a reminder for myself: use this to address #1961 (comment) once this has been merged. Thanks @gaboraranyossy-da.

@gabor-aranyossy gabor-aranyossy force-pushed the test-utils branch 2 times, most recently from 0ad339e to 149ebb6 Compare July 2, 2019 13:26
@mergify mergify bot merged commit 7e7c5f0 into master Jul 2, 2019
@mergify mergify bot deleted the test-utils branch July 2, 2019 15:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@stefanobaghino-da stefanobaghino-da stefanobaghino-da left review comments

@gerolf-da gerolf-da gerolf-da approved these changes

@gleber-da gleber-da gleber-da approved these changes

@rautenrieth-da rautenrieth-da Awaiting requested review from rautenrieth-da

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

Successfully merging this pull request may close these issues.
4 participants
@gabor-aranyossy @gerolf-da @gleber-da @stefanobaghino-da