Skip to content

Conversation

@slinkydeveloper
Copy link
Collaborator

No description provided.

@github-actions
Copy link

Test Results

0 tests  ±0   0 ✅ ±0   0s ⏱️ ±0s
0 suites ±0   0 💤 ±0 
0 files   ±0   0 ❌ ±0 

Results for commit 682f41e. ± Comparison against base commit 1c3deee.

Copy link
Contributor

@tillrohrmann tillrohrmann left a comment

Choose a reason for hiding this comment

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

Thanks for creating an e2e test for the time travel functionality. The changes look good to me. I left a few clarifying questions for my own understanding. +1 for merging once the feature lands.

.containsExactlyInAnyOrder(
"Command: Input",
"Command: Call",
"Notification: CallInvocationId",
Copy link
Contributor

Choose a reason for hiding this comment

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

For my own understanding: When are we returning from val firstMethodResponse = TimeTravelTestCalleeServiceClient.fromContext(ctx).firstMethod()? Before or after the invocation id was decided?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

before, getting the invocation id is an async operation

Comment on lines +129 to +130
"Notification: Run",
"Command: Call",
Copy link
Contributor

Choose a reason for hiding this comment

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

For my own understanding: The notification is allowed to move past Command: Call, right? Is it generated once the run closure completes and persists the value?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yes, that's why containsExactlyInAnyOrder

// Set shouldFail to false so the handler will succeed after time travel
TimeObject.shouldFail.set(false)

// Use the time travel API to trim entry index 2
Copy link
Contributor

Choose a reason for hiding this comment

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

We seem to trim trimIndex and not a fixed index 2.

Comment on lines +127 to +130
"Command: SetState",
"Command: Run",
"Notification: Run",
"Command: Call",
Copy link
Contributor

Choose a reason for hiding this comment

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

For my own understanding: Will the SDK send the commands in creation order (as the awaitables are created in the code)? I would assume so because otherwise it's probably hard to match them on replay with existing journal entries.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Will the SDK send the commands in creation order

yes but the notification order is established by the runtime

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.

3 participants