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

feat: refactor DialogHelper & DialogManager to follow .NET impl #3401

Merged
merged 1 commit into from
Mar 17, 2021

Conversation

stevengum
Copy link
Member

Fixes #3389

Description

Feature parity with microsoft/botbuilder-dotnet#5294

Testing

Existing tests work as expected

@stevengum stevengum requested review from a team as code owners March 16, 2021 00:19
@coveralls
Copy link

coveralls commented Mar 16, 2021

Pull Request Test Coverage Report for Build 661873078

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • 7 unchanged lines in 4 files lost coverage.
  • Overall coverage increased (+0.008%) to 85.074%

Files with Coverage Reduction New Missed Lines %
libraries/botbuilder-dialogs/src/dialogManager.ts 1 91.49%
libraries/botbuilder-lg/src/templateExtensions.ts 1 84.48%
libraries/adaptive-expressions/src/triggerTrees/trigger.ts 2 61.92%
libraries/botbuilder-dialogs/src/dialogHelper.ts 3 95.68%
Totals Coverage Status
Change from base Build 659675778: 0.008%
Covered Lines: 18784
Relevant Lines: 21032

💛 - Coveralls

@@ -49,46 +52,104 @@ export async function runDialog(
}

const dialogSet = new DialogSet(accessor);
dialogSet.telemetryClient = dialog.telemetryClient;
dialogSet.add(dialog);
Copy link
Contributor

Choose a reason for hiding this comment

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

I have some vague recollection of a side-effect of adding a dialog to a dialog set triggering telemetry client updates. Does this reordering break that side-effect?

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for pointing this out, I did some digging and this is what should happen. (I didn't step through the debugger)

PR behavior

  • Add dialog to dialogSet
    • No telemetryClient exists on dialogSet, so dialog.telemetryClient stays the same
  • Update DialogSet to use same telemetry client as dialog

Result: DialogSet and Dialog use the same TelemetryClient

Original behavior

  • Set dialogSet.telemetryClient to dialog.telemetryClient
  • Add dialog to dialogSet
    • dialogSet.telemetryClient exists, so set dialog.telemetryClient to use dialogSet.telemetryClient (same link as above)
    • Since dialogSet uses dialog's telemetry client, an unnecessary setting occurs

Result: DialogSet and Dialog use the same TelemetryClient


However, it seems like there may be a bug in the .NET implementation. If IBotTelemetryClient is found in the TurnState, this will update the telemetryClient on the DialogSet, but not the Dialog. @johnataylor

@stevengum stevengum merged commit 51f347b into main Mar 17, 2021
@stevengum stevengum deleted the stgum/3389 branch March 17, 2021 18:27
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.

port: Johtaylo/dialogrunasync (#5294)
3 participants