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

[ledger-api-test-tool] - Future assertions improvements [KVL-1218] #12294

Merged
merged 8 commits into from
Jan 7, 2022
Merged

[ledger-api-test-tool] - Future assertions improvements [KVL-1218] #12294

merged 8 commits into from
Jan 7, 2022

Conversation

nicu-da
Copy link
Contributor

@nicu-da nicu-da commented Jan 6, 2022

  • Add assertion which is similar to scalatest forAll, which runs multiple test cases in parallel for a sequence of input data, reporting on successes/failures
  • Improve logging for succeedUntil and succeedEventually
  • use the global execution context as the execution context passed to the test cases, to decouple from the execution context used by the akka materializer

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.

@nicu-da nicu-da requested review from a team as code owners January 6, 2022 14:56
@ghost
Copy link

ghost commented Jan 6, 2022

Is the intention to use these new functions in a later changeset?

@nicu-da
Copy link
Contributor Author

nicu-da commented Jan 6, 2022

Is the intention to use these new functions in a later changeset?

Yes, they will be used in the command dedup conformance tests. I wanted to split them up so that it;s easier to review.

ghost
ghost reviewed Jan 6, 2022
View reviewed changes
...-test-tool/src/main/scala/com/daml/ledger/api/testtool/infrastructure/FutureAssertions.scala Outdated

def optionalAssertion(runs: Boolean, description: String)(
assertions: => Future[_]
)(logger: Logger): Future[_] = {
Copy link

Choose a reason for hiding this comment

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

Why not a contextualized logger defined in this file, and a logging context passed in?

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, nit: unneeded braces

...-test-tool/src/main/scala/com/daml/ledger/api/testtool/infrastructure/FutureAssertions.scala Outdated
succeedDeadline: Option[Instant] = None,
)(
test: => Future[V]
)(implicit ec: ExecutionContext, logger: Logger): Future[V] = {
Copy link

Choose a reason for hiding this comment

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

Loggers are not designed to be passed around; you have one per class. If you need context, use ContextualizedLogger and LoggingContext.

...-test-tool/src/main/scala/com/daml/ledger/api/testtool/infrastructure/FutureAssertions.scala
maxInterval: FiniteDuration,
description: String,
)(
test: => Future[V]
Copy link

Choose a reason for hiding this comment

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

Is this actually called twice or is it cached after the first call?

I did a bit of local testing and it seems fine, but I just want to make sure.

Copy link
Contributor

Choose a reason for hiding this comment

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

It would be in principle, but the code paths are mutually exclusive, so it's not.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It will be called up to maxInterval / delay, as we need to retry until it either succeeds or the interval expires

fabiotudone-da
fabiotudone-da reviewed Jan 7, 2022
View reviewed changes
...-test-tool/src/main/scala/com/daml/ledger/api/testtool/infrastructure/FutureAssertions.scala Outdated Show resolved Hide resolved
...-test-tool/src/main/scala/com/daml/ledger/api/testtool/infrastructure/FutureAssertions.scala
maxInterval: FiniteDuration,
description: String,
)(
test: => Future[V]
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be in principle, but the code paths are mutually exclusive, so it's not.

...-test-tool/src/main/scala/com/daml/ledger/api/testtool/infrastructure/FutureAssertions.scala Outdated Show resolved Hide resolved
...-test-tool/src/main/scala/com/daml/ledger/api/testtool/infrastructure/FutureAssertions.scala Show resolved Hide resolved
...-test-tool/src/main/scala/com/daml/ledger/api/testtool/infrastructure/FutureAssertions.scala Outdated Show resolved Hide resolved
...-test-tool/src/main/scala/com/daml/ledger/api/testtool/infrastructure/FutureAssertions.scala Outdated Show resolved Hide resolved
...-test-tool/src/main/scala/com/daml/ledger/api/testtool/infrastructure/FutureAssertions.scala Outdated Show resolved Hide resolved
...-test-tool/src/main/scala/com/daml/ledger/api/testtool/infrastructure/FutureAssertions.scala Outdated

def optionalAssertion(runs: Boolean, description: String)(
assertions: => Future[_]
)(logger: Logger): Future[_] = {
Copy link
Contributor

Choose a reason for hiding this comment

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

Also, nit: unneeded braces

...-test-tool/src/main/scala/com/daml/ledger/api/testtool/infrastructure/FutureAssertions.scala Show resolved Hide resolved
...-test-tool/src/main/scala/com/daml/ledger/api/testtool/infrastructure/FutureAssertions.scala Show resolved Hide resolved
@nicu-da nicu-da requested review from a user and fabiotudone-da January 7, 2022 09:33
fabiotudone-da
fabiotudone-da approved these changes Jan 7, 2022
View reviewed changes
...-test-tool/src/main/scala/com/daml/ledger/api/testtool/infrastructure/FutureAssertions.scala Outdated Show resolved Hide resolved
...-test-tool/src/main/scala/com/daml/ledger/api/testtool/infrastructure/FutureAssertions.scala
Comment on lines +96 to +97
succeedDuration: FiniteDuration,
succeedDeadline: Option[Instant] = None,
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
succeedDuration: FiniteDuration,
succeedDeadline: Option[Instant] = None,
duration: FiniteDuration,
until: Option[Instant] = None,

Also, if you don't need to pass both, consider an Either.

@nicu-da @fabiotudone-da
Update ledger/ledger-api-test-tool/src/main/scala/com/daml/ledger/api…
aceb7d6 
…/testtool/infrastructure/FutureAssertions.scala

Co-authored-by: fabiotudone-da <[email protected]>
@nicu-da
Copy link
Contributor Author

nicu-da commented Jan 7, 2022

I'll be merging this to unblock my follow up PRs and will fix any other comments in a separate PR.

@nicu-da nicu-da enabled auto-merge (squash) January 7, 2022 10:05
@nicu-da nicu-da merged commit 1d258a1 into main Jan 7, 2022
@nicu-da nicu-da deleted the nicuda/kvl-1218/future_assertions branch January 7, 2022 11:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fabiotudone-da fabiotudone-da fabiotudone-da approved these changes

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

Successfully merging this pull request may close these issues.
2 participants
@nicu-da @fabiotudone-da