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

DialogTestClient constructor updates and XUnitOutputMiddleware renames #2176

Merged
merged 1 commit into from
Jul 3, 2019

Conversation

gabog
Copy link
Contributor

@gabog gabog commented Jul 3, 2019

Renamed XUnitOutputMiddleware to XUnitDialogTestLogger (so its name is closer to js).
Added some spaces to the XUnitDialogTestLogger output so it reads better.
Removed callback constructor parameter from DialogTestClient (it relies on an internal DialogTurnResult and that's fishy, we will need to find a better solution in the future to support this).
Added optional ConversationState parameter to the DialogTestClient constructor and exposed a property to allow people to inspect it or access it.

…s closer to js).

Added some spaces to the XUnitDialogTestLogger  output so it reads better.
Removed callback constructor parameter from DialogTestClient (it relies on an internal DialogTurnResult, we will need to find a better solution in the future to support this).
Added optional ConversationState parameter to the constructor and exposed a property to allow people to inspect it or access it.
@gabog gabog requested a review from johnataylor July 3, 2019 06:46
@fuselabs
Copy link
Collaborator

fuselabs commented Jul 3, 2019

✔️ No Binary Compatibility issues for Microsoft.Bot.Builder.dll compared against version 4.3.1
✔️ No Binary Compatibility issues for Microsoft.Bot.Builder.AI.Luis.dll compared against version 4.3.1
✔️ No Binary Compatibility issues for Microsoft.Bot.Builder.AI.QnA.dll compared against version 4.3.1
✔️ No Binary Compatibility issues for Microsoft.Bot.Builder.ApplicationInsights.dll compared against version 4.3.1
✔️ No Binary Compatibility issues for Microsoft.Bot.Builder.Azure.dll compared against version 4.3.1
✔️ No Binary Compatibility issues for Microsoft.Bot.Builder.Dialogs.dll compared against version 4.3.1
✔️ No Binary Compatibility issues for Microsoft.Bot.Builder.TemplateManager.dll compared against version 4.3.1
✔️ No Binary Compatibility issues for Microsoft.Bot.Configuration.dll compared against version 4.3.1
✔️ No Binary Compatibility issues for Microsoft.Bot.Connector.dll compared against version 4.3.1
✔️ No Binary Compatibility issues for Microsoft.Bot.Schema.dll compared against version 4.3.1

@coveralls
Copy link
Collaborator

Pull Request Test Coverage Report for Build 68661

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.004%) to 77.029%

Totals Coverage Status
Change from base Build 68627: 0.004%
Covered Lines: 4946
Relevant Lines: 6421

💛 - Coveralls

/// Gets the latest <see cref="ConversationState"/> for <see cref="DialogTestClient"/>.
/// </summary>
/// <value>A <see cref="ConversationState"/> instance for the current test client.</value>
public ConversationState ConversationState { get; }
Copy link
Member

Choose a reason for hiding this comment

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

why a public property?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In some tests, they may need to write some preconditions in ConversationState, I looked at some of the VA dialogs and other projects and in some scenarios they cache user information and other info when a conversation starts that's then used by the dialogs.

@gabog gabog merged commit 085bff0 into master Jul 3, 2019
@stevengum stevengum deleted the gabog/DialogTestClientUpdates branch July 3, 2019 22:40
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.

None yet

4 participants