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

Fix Random thread safety and control random seed to improve testability #4429

Merged
merged 13 commits into from
Aug 19, 2020

Conversation

Danieladu
Copy link
Contributor

@Danieladu Danieladu commented Aug 11, 2020

Fixes #4361

Description

This PR aims to accept a "Microsoft.SetTestOptions" event, and apply the options to SDK.

Specific Changes

  1. Add setTestOptions event
  2. Save TestOptions into the conversation state. Including the Random seed and random value.
  3. Achieve the state from "dialogContext.State", and apply it to SDK code.
  4. Adjust random generator code to use the random in "TestOptions".

How to test

  1. Start project "Microsoft.Bot.Builder.TestBot.Json"
  2. Input 26 to see the dialog output.

Progress

  • Adaptive-expression
  • LG Evaluator/Expander
  • Random selector

@chrimc62
Copy link
Contributor

chrimc62 commented Aug 11, 2020

Good progress. A few things:

  1. Specifying a single random with a seed is useful, but even more useful is if you can specify the single number to return every place you want a random number. The reason is that it makes for a much more stable test that is not dependent on how many times you call random. Both should be in your options.
  2. To set the test options you should be using a custom setTestOptions event using an emitEvent action where eventName = "setTestOptions" and eventValue={ randomSeed: , randomValue: }. This is the event that middle ware should pick up and make available ideally through conversation.testOptions memory. This is a general mechanism where over time we can add more information to the object and either bake in uses like here or even have templates output extra information if you want to do custom testing stuff. #Resolved

@Danieladu Danieladu requested a review from chrimc62 August 12, 2020 04:34
@chrimc62
Copy link
Contributor

Good changes for the RandomValue except for min/max adjustment.
Ideally we should not be changing adaptive at all. The event is a custom one with an arbitrary payload. The only place the event should be consumed is in middle ware that is a part of adaptive.testing. The middle ware should detect the event and put the eventValue into conversation.testOptions.


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

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
Copy link
Contributor Author

Danieladu commented Aug 13, 2020

Good changes for the RandomValue except for min/max adjustment.
Ideally we should not be changing adaptive at all. The event is a custom one with an arbitrary payload. The only place the event should be consumed is in middle ware that is a part of adaptive.testing. The middle ware should detect the event and put the eventValue into conversation.testOptions.

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

Currently, the only thing that is blocked on my side is how to catch the "emitEvent" event in middleware.
I have made some investigations about the "emitEvent" action, and does not find any real event was emitted, so, in this way, I can not add a subscriber in a middleware. May you provide some suggestions about this one? Thanks very much.

#Resolved

@Danieladu Danieladu requested a review from chrimc62 August 17, 2020 08:24
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:

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.

Fix Random thread safety and control random seed to improve testability
4 participants