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.

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

ledger-api-test-tool: Add assertions for deduplication period, use them in existing tests [KVL-1220] #12250

Merged
merged 10 commits into from
Jan 17, 2022

Conversation

hubert-da
Copy link
Collaborator

@hubert-da hubert-da commented Jan 3, 2022

Tested manually by flipping the comparisons.

Pull Request Checklist

  • Read and understand the contribution guidelines
  • Include appropriate tests
  • Set a descriptive title and thorough description
  • Add a reference to the issue this PR will solve, if appropriate
  • Include changelog additions in one or more commit message bodies between the CHANGELOG_BEGIN and CHANGELOG_END tags
  • Normal production system change, include purpose of change in description
  • If you mean to change the status of a component, please make sure you keep the Component Status page up to date.

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.

@hubert-da hubert-da requested a review from nicu-da January 3, 2022 16:05
@garyverhaegen-da
Copy link
Contributor

/azp run

@azure-pipelines
Copy link
Contributor

Azure Pipelines successfully started running 1 pipeline(s).

@hubert-da hubert-da force-pushed the hubert-da/assert-deduplication-period branch 3 times, most recently from 1336d12 to 000c2ac Compare January 12, 2022 08:13
@hubert-da hubert-da requested review from a team and andreaslochbihler-da January 12, 2022 12:39
@hubert-da hubert-da marked this pull request as ready for review January 12, 2022 12:40
@hubert-da
Copy link
Collaborator Author

Still working on making compatibility tests green. All other tests were green, so moving to "In Review".

} yield {
assertDeduplicationOffset(
Ref.HexString.assertFromString(response.offset.getAbsolute),
response,
Copy link
Contributor

Choose a reason for hiding this comment

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

Note to self: This test assumes inclusive deduplication offsets. Will need to change when we switch to exclusive dedup offsets.

…hem in existing tests [KVL-1220]

CHANGELOG_BEGIN
CHANGELOG_END
CHANGELOG_BEGIN
CHANGELOG_END
CHANGELOG_BEGIN
CHANGELOG_END
CHANGELOG_BEGIN
CHANGELOG_END

run-full-compat: true
…ssertCompletionIsDefined` to `assertDefined`

CHANGELOG_BEGIN
CHANGELOG_END

run-full-compat: true
@hubert-da hubert-da force-pushed the hubert-da/assert-deduplication-period branch from a5ab7bc to e45d445 Compare January 13, 2022 08:25
CHANGELOG_BEGIN
CHANGELOG_END

run-full-compat: true
Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

The code looks fine, but I will leave it to @andreaslochbihler-da to review the logic.

CHANGELOG_BEGIN
CHANGELOG_END

run-full-compat: true
)
}
} else {
// Let t2 (completionReceiveTime) be the time when the application received the completion offset that specifies the deduplication offset off1.
Copy link
Contributor

Choose a reason for hiding this comment

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

What is off1? How does off1 relate to reportedOffset?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

What is off1?

As you can see in the line below, that's an offset for the completed submission (it would be the requested deduplication offset if it was a test for duplication with a deduplication offset, at least while the deduplication offsets are inclusive). That's almost one-to-one from the acceptance criteria.

How does off1 relate to reportedOffset?

IMHO it's unrelated, at least while the reported offsets are inclusive.

Comment on lines 68 to 76
// Let t2 (completionReceiveTime) be the time when the application received the completion offset that specifies the deduplication offset off1.
// Let t1 (previousSubmissionSendTime) be the time when the application sent off the submission that completed at off1.
// Then t2 - t1 >= requested deduplication duration.
assert(
requestedDuration <= Duration
.between(previousSubmissionSendTime, completionReceiveTime),
s"The requested deduplication duration $requestedDeduplicationDuration was greater than the duration between sending the previous submission and receiving the next completion.",
)
Future.unit
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks like dead code right now because all use cases of this method should have the status code ALREADY_EXISTS, so we never end up in this branch.

This assertion still does not make sense, because previousSubmissionSendTime refers to some arbitrary offset that was submitted previously by the test, which is therefore independent of reportedOffset. What would be the correct thing to do here is to remember a "submission time" for all offsets that were previously generated, and then extract the time for reportedOffset from this history and use that as t1.

Since all this is irrelevant for this PR, I recommend to just remove this check and put a placeholder like fail("This case is not yet supported by this assertion") here so that nobody can accidentally run into this problem.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Unfortunately, the entire duration -> offset conversion case is dead code, as this test is ignored for bazel test //ledger/ledger-api-test-tool-on-canton:conformance-test, as, among the others, Canton does not expose feature descriptors. So, to test it (locally), I had to remove the exclusion and comment out the code for other deduplication period support cases.

But I agree, I removed this check for now.

CHANGELOG_BEGIN
CHANGELOG_END
CHANGELOG_BEGIN
CHANGELOG_END
@hubert-da hubert-da enabled auto-merge (squash) January 17, 2022 10:11
@hubert-da hubert-da disabled auto-merge January 17, 2022 10:29
CHANGELOG_BEGIN
CHANGELOG_END
@hubert-da hubert-da enabled auto-merge (squash) January 17, 2022 10:39
@hubert-da hubert-da merged commit 553ce22 into main Jan 17, 2022
@hubert-da hubert-da deleted the hubert-da/assert-deduplication-period branch January 17, 2022 11:46
azure-pipelines bot pushed a commit that referenced this pull request Jan 19, 2022
This PR has been created by a script, which is not very smart
and does not have all the context. Please do double-check that
the version prefix is correct before merging.

@garyverhaegen-da is in charge of this release.

Commit log:
```
d0813e6 Factor out version check in docs cron (#12459)
ca3b11c Upgrade H2 to v2.1.210 (#12461)
eb2281d Move more docs post processing to non-sphinx docs (#12463)
83b99ba [ledger-api-test-tool] - Add test cases for invalid deduplication periods [KVL-1222] (#12434)
995c8bd Unpin the `self-signed` resolution (#12446)
01b0b76 Support split releases in the docs cronjob (#12456)
4fe6e53 [User management] Persistence with caching (#12344)
f07791e Upgrade typescript and typedoc (#12457)
2640bc6 user management: require appropriate scope in user access tokens (#12428)
0c13a4f Error handling for User Management exposed via daml-script (#12416)
530509a Add sanity check in quickstart-java assistant test (#12455)
2500d4f Make quickstart-java template work with canton sandbox. (#12453)
25ddfcd LF: Add tests for evaluation order on fetches (#12411)
430f86b Make skeleton template work with canton sandbox (#12451)
4af48bb Split channel configuration from LedgerClientConfiguration (#12433)
b988a3c Use --sandbox-kv in create-daml-app release tests. (#12450)
27bfd40 Split Daml cron executable into separate modules (#12449)
b2a7f9e Change default sandbox to canton (#12438)
ab16a54 update NOTICES file (#12447)
e6fe718 trigger split-release (#12444)
69a8b67 Fix reported vulnerabilities (#12443)
b996e5e Support user tokens in Daml Script over JSON API (#12368)
36a93ef try to fix paths in split-release (#12442)
5922df6 trigger split-release (#12436)
98c9461 Remove usages of `Flowable.fromFuture` and `Single.fromFuture` (#12425)
502b21f split-release: publish temp files to artifactory (#12435)
bf39f48 Upgrade vsce and markdown-it (#12431)
e5b400d [Sandbox-on-X unit test] PrepareSubmissionSpec (#12383)
5a7b2ba ledger-api - Rename error method to invalidDeduplicationPeriod [KVL-1222] (#12424)
553ce22 ledger-api-test-tool: Add assertions for deduplication period, use them in existing tests [KVL-1220] (#12250)
29ba8e6 Drop sandbox classic from the SDK tarball (#12410)
93aa5ab Upgrade to NodeJS 14 (#12429)
a0aee0f Rename daml sandbox to daml sandbox-kv (#12394)
8baaf72 Migrate migration tests to Sandbox on X (#12407)
903f473 kvutils - Handle deduplication periods specified by offsets > ledger end [KVL-1222] (#12423)
6a3ef80 [Sandbox-on-X unit test] SequencerStateSpec [DPP-841] (#12380)
6d6b433 Bump shelljs from 0.8.4 to 0.8.5 in /language-support/ts/packages (#12426)
84d413e Release SDK 1.18.1 (#12415)
952a939 Bump follow-redirects from 1.14.0 to 1.14.7 in /navigator/frontend (#12418)
d9806f2 [Sandbox-on-X unit test] SequenceSpec [DPP-841] (#12362)
0eb500d update NOTICES file (#12413)
27b8ca0 Publish Sandbox-on-X app lib jar (#12414)
5716d99 Disable printer sharing (#12408)
33bc6fe LF: Wrongly typed local contract is not a internal error (#12406)
ce62491 Fix damlc warnings & compile flags (#12404)
b21c960 Make Daml Script user mgmt tests order independent (#12409)
9cc2a6e Support UserManagementService on the Java bindings (#12403)
ea2d4bc LF: factorize typeAndCompile in Speedy tests (#12402)
8663b7b kvutils: Remove unused proto import (#12405)
2005068 Drop `platform-version` support from `daml.yaml` (#12397)
82bdfa5 ledger-api-auth: remove SupportedJWTPayload (#12343)
d04ea6c Rerelease 1.18.1 RC (#12395)
f94e24b Bump follow-redirects from 1.13.3 to 1.14.7 (#12400)
c027a61 Increase timeout on an integration-test query. (#12399)
846bc38 Switch ledger-api-test-tool compat tests to sandbox on x (#12393)
8d31e93 Drop unused vars from bazel-haskell-deps (#12387)
e34ac20 offboarding Akshay (#12396)
bb8f455 Add test coverage for validateTokenParties (#12370)
6aa9409 split-releases: gcs accounts for assembly & canton (#12373)
dfe22b8 Release SDK 1.18.1 RC 2 (#12385)
e9e673c Drop scenario compat helpers (#12389)
b7ded08 Publish sandbox on x as a standalone JAR (#12386)
1193f5d Changes to disable multiplexing for websockets (#12384)
903df7c Bump flyway to version 8.4.1 (#12379)
8e9f993 Convert storage backend tests to AnyFlatSuite (#12278)
9513c64 Fix compat test exclusions (#12378)
a90122e Fix crash when stream closed without producing an element [DPP-853] (#12363)
ae547ef kvutils/app: Add tests for health checks when starting with the Runner. (#12376)
bfdf6e7 kvutils/app: Create a test case for the Runner. [KVL-663] (#12374)
249ba22 Be explicit in the usage of feature descriptors (#12356)
0a8568f Attempt to make ConcurrencyLimiterSpec less flaky (#12371)
9c77a3c kvutils: Remove the DAR upload parameters from the runner. (#12366)
da55bd4 Simplify sphinx -Werror (#12372)
09013eb Add eager timeouts to the polling functions used in daml start and assistant integration tests. (#12361)
8bc5804 Add feature descriptor for the support for max deduplication duration [KVL-1218] (#12355)
ba2af23 Drop kvutils export (#12359)
c6e5fc8 [Sandbox-on-X] ConflictCheckWithCommittedSpec (#12358)
62b61c0 ledger api auth: polish error messages (#12350)
31bc1a7 Use nanos for deduplication duration comparison [KVL-1218] (#12360)
8bc5ff0 Clarify unhandled exception error message (#12353)
e5dcea1 design guidelines for JSON API endpoints (#12340)
606019e local import cleanup in json-api (#12335)
ba422bf resources: Add more test coverage for parallel acquisition. (#12357)
30300fd LF: Revert #7742 (#12354)
631aa5c Create a new split release (#12351)
8a2449b TagWithLedgerEndSpec (#12346)
b845587 Make create-daml-app setup idempotent (#12320)
98b7ad7 [SoX unit testing] Refactored for testing [DPP-841] (#12337)
a7c51ef version-service - Move command deduplication feature descriptors into the experimental features [kvl-1218] (#12318)
4211557 Add string interning benchmark (#12237)
fafb86b Add static time feature flag and test tool coverage [DPP-849] (#12336)
7ec55d7 LF: Cache field index inside structural record projection builtin (#12338)
4deee3f Add get & delete user endpoints (#12332)
79428bb Add artifactory & SDK EE artifacts to split-release process (#12345)
e4dbde3 [JSON-API] Add list users endpoint (#12323)
50deee3 [ledger-api-test-tool] - Merge KVCommandDeduplicationIT and CommandDeduplicationIT (#12257)
c7c86d2 LF: Benchmark for structural record projection (#12329)
d653ca9 Release SDK 1.18.1 RC (#12339)
c8753b5 Switch to H2 BINARY LARGE OBJECT to work around a limitation with BYTEA (#12334)
fdee5cb update NOTICES file (#12342)
50de6e3 [JSON-API] HOCON config json api (#12236)
9f5a2f9 Fix terraform (#12333)
face7d3 Remove unused line as reported in #12325 (#12331)
37680be Add sha256sum to dev-env (#12328)
83d2d98 Release SDK v2.0.0-snapshot.20220110.8812.0.3a08380b (#12327)
ca33990 Use canton port file in daml start and tests. (#12324)
c8bbeed Switch to daml-data gcs bucket for split-releases (#12322)
```
Changelog:
```
[Scala bindings] The channel configuration has been split from the
LedgerClientConfiguration class. Provide the gRPC channel specific
configuration separately or use a builder. The channel configuration
no longer overrides the builder.
[Java bindings] The Single resulting from calling several of the services
exposed by the bindings are no longer blocking. For further details, see
#12193.
[Java bindings] User management service client added

- [HTTP-JSON] Added endpoints:
    - /user/delete that if called with GET will delete the current user & with POST will delete the user specified via the payload
    - /user that if called with POST will now return user info about the user specified via the payload

- [HTTP-JSON] Added an endpoint /users which returns the available users on the ledger.

[ledger-api-test-tool] - Merge command deduplication ledger API tests (`KVCommandDeduplicationIT`, `CommandDeduplicationIT`) into a single suite(`CommandDeduplicationIT`) which uses feature descriptors to handle different participant behaviors
JSON-API can now be started supplying a HOCON application config file using the `--config` option.
All CLI flags except `logging` and `non-repudiation` one's are now deprecated and will be cleaned up in some future releases.
```

CHANGELOG_BEGIN
CHANGELOG_END
cocreature pushed a commit that referenced this pull request Jan 19, 2022
This PR has been created by a script, which is not very smart
and does not have all the context. Please do double-check that
the version prefix is correct before merging.

@garyverhaegen-da is in charge of this release.

Commit log:
```
d0813e6 Factor out version check in docs cron (#12459)
ca3b11c Upgrade H2 to v2.1.210 (#12461)
eb2281d Move more docs post processing to non-sphinx docs (#12463)
83b99ba [ledger-api-test-tool] - Add test cases for invalid deduplication periods [KVL-1222] (#12434)
995c8bd Unpin the `self-signed` resolution (#12446)
01b0b76 Support split releases in the docs cronjob (#12456)
4fe6e53 [User management] Persistence with caching (#12344)
f07791e Upgrade typescript and typedoc (#12457)
2640bc6 user management: require appropriate scope in user access tokens (#12428)
0c13a4f Error handling for User Management exposed via daml-script (#12416)
530509a Add sanity check in quickstart-java assistant test (#12455)
2500d4f Make quickstart-java template work with canton sandbox. (#12453)
25ddfcd LF: Add tests for evaluation order on fetches (#12411)
430f86b Make skeleton template work with canton sandbox (#12451)
4af48bb Split channel configuration from LedgerClientConfiguration (#12433)
b988a3c Use --sandbox-kv in create-daml-app release tests. (#12450)
27bfd40 Split Daml cron executable into separate modules (#12449)
b2a7f9e Change default sandbox to canton (#12438)
ab16a54 update NOTICES file (#12447)
e6fe718 trigger split-release (#12444)
69a8b67 Fix reported vulnerabilities (#12443)
b996e5e Support user tokens in Daml Script over JSON API (#12368)
36a93ef try to fix paths in split-release (#12442)
5922df6 trigger split-release (#12436)
98c9461 Remove usages of `Flowable.fromFuture` and `Single.fromFuture` (#12425)
502b21f split-release: publish temp files to artifactory (#12435)
bf39f48 Upgrade vsce and markdown-it (#12431)
e5b400d [Sandbox-on-X unit test] PrepareSubmissionSpec (#12383)
5a7b2ba ledger-api - Rename error method to invalidDeduplicationPeriod [KVL-1222] (#12424)
553ce22 ledger-api-test-tool: Add assertions for deduplication period, use them in existing tests [KVL-1220] (#12250)
29ba8e6 Drop sandbox classic from the SDK tarball (#12410)
93aa5ab Upgrade to NodeJS 14 (#12429)
a0aee0f Rename daml sandbox to daml sandbox-kv (#12394)
8baaf72 Migrate migration tests to Sandbox on X (#12407)
903f473 kvutils - Handle deduplication periods specified by offsets > ledger end [KVL-1222] (#12423)
6a3ef80 [Sandbox-on-X unit test] SequencerStateSpec [DPP-841] (#12380)
6d6b433 Bump shelljs from 0.8.4 to 0.8.5 in /language-support/ts/packages (#12426)
84d413e Release SDK 1.18.1 (#12415)
952a939 Bump follow-redirects from 1.14.0 to 1.14.7 in /navigator/frontend (#12418)
d9806f2 [Sandbox-on-X unit test] SequenceSpec [DPP-841] (#12362)
0eb500d update NOTICES file (#12413)
27b8ca0 Publish Sandbox-on-X app lib jar (#12414)
5716d99 Disable printer sharing (#12408)
33bc6fe LF: Wrongly typed local contract is not a internal error (#12406)
ce62491 Fix damlc warnings & compile flags (#12404)
b21c960 Make Daml Script user mgmt tests order independent (#12409)
9cc2a6e Support UserManagementService on the Java bindings (#12403)
ea2d4bc LF: factorize typeAndCompile in Speedy tests (#12402)
8663b7b kvutils: Remove unused proto import (#12405)
2005068 Drop `platform-version` support from `daml.yaml` (#12397)
82bdfa5 ledger-api-auth: remove SupportedJWTPayload (#12343)
d04ea6c Rerelease 1.18.1 RC (#12395)
f94e24b Bump follow-redirects from 1.13.3 to 1.14.7 (#12400)
c027a61 Increase timeout on an integration-test query. (#12399)
846bc38 Switch ledger-api-test-tool compat tests to sandbox on x (#12393)
8d31e93 Drop unused vars from bazel-haskell-deps (#12387)
e34ac20 offboarding Akshay (#12396)
bb8f455 Add test coverage for validateTokenParties (#12370)
6aa9409 split-releases: gcs accounts for assembly & canton (#12373)
dfe22b8 Release SDK 1.18.1 RC 2 (#12385)
e9e673c Drop scenario compat helpers (#12389)
b7ded08 Publish sandbox on x as a standalone JAR (#12386)
1193f5d Changes to disable multiplexing for websockets (#12384)
903df7c Bump flyway to version 8.4.1 (#12379)
8e9f993 Convert storage backend tests to AnyFlatSuite (#12278)
9513c64 Fix compat test exclusions (#12378)
a90122e Fix crash when stream closed without producing an element [DPP-853] (#12363)
ae547ef kvutils/app: Add tests for health checks when starting with the Runner. (#12376)
bfdf6e7 kvutils/app: Create a test case for the Runner. [KVL-663] (#12374)
249ba22 Be explicit in the usage of feature descriptors (#12356)
0a8568f Attempt to make ConcurrencyLimiterSpec less flaky (#12371)
9c77a3c kvutils: Remove the DAR upload parameters from the runner. (#12366)
da55bd4 Simplify sphinx -Werror (#12372)
09013eb Add eager timeouts to the polling functions used in daml start and assistant integration tests. (#12361)
8bc5804 Add feature descriptor for the support for max deduplication duration [KVL-1218] (#12355)
ba2af23 Drop kvutils export (#12359)
c6e5fc8 [Sandbox-on-X] ConflictCheckWithCommittedSpec (#12358)
62b61c0 ledger api auth: polish error messages (#12350)
31bc1a7 Use nanos for deduplication duration comparison [KVL-1218] (#12360)
8bc5ff0 Clarify unhandled exception error message (#12353)
e5dcea1 design guidelines for JSON API endpoints (#12340)
606019e local import cleanup in json-api (#12335)
ba422bf resources: Add more test coverage for parallel acquisition. (#12357)
30300fd LF: Revert #7742 (#12354)
631aa5c Create a new split release (#12351)
8a2449b TagWithLedgerEndSpec (#12346)
b845587 Make create-daml-app setup idempotent (#12320)
98b7ad7 [SoX unit testing] Refactored for testing [DPP-841] (#12337)
a7c51ef version-service - Move command deduplication feature descriptors into the experimental features [kvl-1218] (#12318)
4211557 Add string interning benchmark (#12237)
fafb86b Add static time feature flag and test tool coverage [DPP-849] (#12336)
7ec55d7 LF: Cache field index inside structural record projection builtin (#12338)
4deee3f Add get & delete user endpoints (#12332)
79428bb Add artifactory & SDK EE artifacts to split-release process (#12345)
e4dbde3 [JSON-API] Add list users endpoint (#12323)
50deee3 [ledger-api-test-tool] - Merge KVCommandDeduplicationIT and CommandDeduplicationIT (#12257)
c7c86d2 LF: Benchmark for structural record projection (#12329)
d653ca9 Release SDK 1.18.1 RC (#12339)
c8753b5 Switch to H2 BINARY LARGE OBJECT to work around a limitation with BYTEA (#12334)
fdee5cb update NOTICES file (#12342)
50de6e3 [JSON-API] HOCON config json api (#12236)
9f5a2f9 Fix terraform (#12333)
face7d3 Remove unused line as reported in #12325 (#12331)
37680be Add sha256sum to dev-env (#12328)
83d2d98 Release SDK v2.0.0-snapshot.20220110.8812.0.3a08380b (#12327)
ca33990 Use canton port file in daml start and tests. (#12324)
c8bbeed Switch to daml-data gcs bucket for split-releases (#12322)
```
Changelog:
```
[Scala bindings] The channel configuration has been split from the
LedgerClientConfiguration class. Provide the gRPC channel specific
configuration separately or use a builder. The channel configuration
no longer overrides the builder.
[Java bindings] The Single resulting from calling several of the services
exposed by the bindings are no longer blocking. For further details, see
#12193.
[Java bindings] User management service client added

- [HTTP-JSON] Added endpoints:
    - /user/delete that if called with GET will delete the current user & with POST will delete the user specified via the payload
    - /user that if called with POST will now return user info about the user specified via the payload

- [HTTP-JSON] Added an endpoint /users which returns the available users on the ledger.

[ledger-api-test-tool] - Merge command deduplication ledger API tests (`KVCommandDeduplicationIT`, `CommandDeduplicationIT`) into a single suite(`CommandDeduplicationIT`) which uses feature descriptors to handle different participant behaviors
JSON-API can now be started supplying a HOCON application config file using the `--config` option.
All CLI flags except `logging` and `non-repudiation` one's are now deprecated and will be cleaned up in some future releases.
```

CHANGELOG_BEGIN
CHANGELOG_END

Co-authored-by: Azure Pipelines Daml Build <[email protected]>
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