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 #4361

Closed
chrimc62 opened this issue Jul 31, 2020 · 0 comments · Fixed by #4429
Closed

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

chrimc62 opened this issue Jul 31, 2020 · 0 comments · Fixed by #4429
Assignees
Labels
Area: Adaptive expression The issue is related to Adaptive expressions Area: Adaptive The issue is related to Adaptive dialogs

Comments

@chrimc62
Copy link
Contributor

Problem

Our usage of Random is not thread safe and when writing tests, you currently need to test for all random responses. This makes it harder to write tests and adds minimally to the test coverage. One method in particular is to take a transcript and turn it into declarative test to make sure the same path gets executed. This does not work if there is any variation in .lg templates or if the random selector is used.

Context

Where random is used:

  1. Rand function in adaptive-expressions. This uses a single static generator which is not thread safe. There is no way to set the seed.
  2. .lg randomization in Evaluator and Expander. Each of these creates a new Random object on every call. Creating a Random object is expensive, so this is not ideal and there is no way to set the seed.
  3. Random selector creates a new Random for each instance and does support setting the seed through the schema. This is also potentially an issue with thread safety.

Solution

  1. Create middleware in the adaptive test package to respond to a "Microsoft.SetTestOptions" event by taking the payload and putting it into conversation.TestOptions.
  2. Everywhere we use random numbers (adaptive expressions, .lg, random selector) check for conversationTestOptions.RandomValue and if present use that in place of calling random.

To support recording a transcript with the random value set we also need to add this to the emulator which will allow emitting the test event while specifying the RandomValue. microsoft/BotFramework-Emulator#2174

With these in place a transcript will contain the Microsoft.SetTraceOptions trace that can be included in a declarative test to emit the same activity so that the test script will have the same predictable behavior.

This solution will enable us to control the random choices made so that we can ensure that a test will run consistently. Once we have made these improvements, we will also need to add the middle ware to the default Composer runtime.

[enhancement]

@gabog gabog added Area: Adaptive The issue is related to Adaptive dialogs Area: Adaptive expression The issue is related to Adaptive expressions and removed Adaptive labels Aug 12, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: Adaptive expression The issue is related to Adaptive expressions Area: Adaptive The issue is related to Adaptive dialogs
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants