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

[PORT] Add LG telemetry #2451

Merged
merged 20 commits into from
Aug 5, 2020
Merged

[PORT] Add LG telemetry #2451

merged 20 commits into from
Aug 5, 2020

Conversation

Danieladu
Copy link
Contributor

Fixes #1864

Add LG telemetry

@coveralls
Copy link

coveralls commented Jun 30, 2020

Pull Request Test Coverage Report for Build 154271

  • 12 of 13 (92.31%) changed or added relevant lines in 5 files are covered.
  • 2 unchanged lines in 2 files lost coverage.
  • Overall coverage increased (+0.1%) to 82.811%

Changes Missing Coverage Covered Lines Changed/Added Lines %
libraries/botbuilder-dialogs-adaptive/src/input/inputDialog.ts 6 7 85.71%
Files with Coverage Reduction New Missed Lines %
libraries/botbuilder-dialogs-adaptive/src/actions/beginSkill.ts 1 64.1%
libraries/botbuilder-dialogs-adaptive/src/input/inputDialog.ts 1 78.08%
Totals Coverage Status
Change from base Build 154236: 0.1%
Covered Lines: 14737
Relevant Lines: 16916

💛 - Coveralls

Copy link
Contributor

@chrimc62 chrimc62 left a comment

Choose a reason for hiding this comment

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

🕐

@Danieladu Danieladu requested a review from chrimc62 July 8, 2020 08:27
Copy link
Member

@stevengum stevengum 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 the changes @Danieladu. Can you add some unit tests for them?

@tomlm these telemetry client calls in BeginSkill and HttpRequest don't exist in the .NET implementation. Can you review this PR and determine if we want to port them over to .NET?

HttpRequest.ReplaceJTokenRecursivelyAsync()
BeginSkill.BeginDialogAsync()

@stevengum
Copy link
Member

@chrimc62, should the telemetryClient calls in BeginSkill and HttpRequest be ported back to C#?

@chrimc62
Copy link
Contributor

Yes if they are here only.


In reply to: 655837544 [](ancestors = 655837544)

Copy link
Contributor

@chrimc62 chrimc62 left a comment

Choose a reason for hiding this comment

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

:shipit:

Copy link
Member

@stevengum stevengum left a comment

Choose a reason for hiding this comment

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

@Danieladu unit tests? 🕐

@Danieladu
Copy link
Contributor Author

Danieladu commented Jul 24, 2020

@Danieladu unit tests? 🕐

Sorry for the delay. i hope I can add some unit tests in the next few days.
BTW, I found there is still no unit test for C# side for all telemetry, and test the telemetry is very hard because it is just a small part of the dialog flow evaluation. So, could you give some advices that how to test the telemetry? Hmmm, currently, I have no idea about this part.

@boydc2014
Copy link
Contributor

@Danieladu unit tests? 🕐

Sorry for the delay. i hope I can add some unit tests in the next few days.
BTW, I found there is still no unit test for C# side for all telemetry, and test the telemetry is very hard because it is just a small part of the dialog flow evaluation. So, could you give some advices that how to test the telemetry? Hmmm, currently, I have no idea about this part.

maybe mock a telemetry client and assert the mock client is been called with expected data?

@Danieladu Danieladu requested a review from a team as a code owner July 25, 2020 02:43
@Danieladu Danieladu requested a review from stevengum July 25, 2020 03:16
@stevengum stevengum dismissed their stale review July 28, 2020 05:56

out of date

Copy link
Member

@stevengum stevengum left a comment

Choose a reason for hiding this comment

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

Please see feedback for guidance on authoring tests along with examples.


it('basic telemetry test', () => {
const mockDialog = new MockDialog();
mockDialog.telemetryClient = new MockTelemetryClient();
Copy link
Member

Choose a reason for hiding this comment

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

Since a Dialog will automatically have at least a NullTelemetryClient, you can use sinon to stub out the trackEvent method on dialogInstance.telemetryClient.

const { stub } = require('sinon');
const trackEventStub = stub(mockDialog.telemetryClient, 'trackEvent');

// Later after the method that calls trackEvent finishes
assert(trackEventStub.called);

And use a captureAction to verify the stub's input.

There are also examples in botframeworkAdapter.test.js and the skillDialog.test.js files. The SkillDialog tests may be more helpful and will show an example closer to what @boydc2014 suggested. Additionally, the tests for SkillDialog in JS are extremely close to the tests in .NET (see SkillDialogTests.cs), so that may help in authoring tests for the LG Telemetry changes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have added some telemetry tests in skillDialog.test.js. Please take a look. Thanks.

Copy link
Member

Choose a reason for hiding this comment

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

@Danieladu the updates to the BeginSkill tests looks great, can you move createTelemetryClientAndStub() to a utilities file since that capture action is self contained and then use it for the other tests as well?

@Danieladu Danieladu requested a review from stevengum July 28, 2020 10:18
@stevengum stevengum added the R10 Release 10 - August 17th, 2020 label Jul 30, 2020
return [skillClient, postActivityStub];
}

function createtelemetryClientAndStub(captureTelemetryAction) {
Copy link
Member

Choose a reason for hiding this comment

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

Can you make the function name pascal cased? createTelemetryClientAndStub

Copy link
Member

@stevengum stevengum left a comment

Choose a reason for hiding this comment

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

The test changes look good so far, per my comment can you apply do some small refactoring to reuse the test logic to add tests for the other changes in this PR?

@stevengum stevengum added the changes required Reviews has requested changes to be made before approval label Jul 31, 2020
@Danieladu Danieladu requested a review from stevengum August 1, 2020 03:47
@Danieladu Danieladu removed the changes required Reviews has requested changes to be made before approval label Aug 3, 2020
@stevengum stevengum merged commit afef785 into master Aug 5, 2020
@stevengum stevengum deleted the hond/telemetry branch August 5, 2020 20:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
R10 Release 10 - August 17th, 2020
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[PORT] Add LG telemetry
5 participants