-
Notifications
You must be signed in to change notification settings - Fork 283
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
Adaptive TestRunner refactoring #3055
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A couple of quick suggestions, otherwise this looks good!
adapter.use(middleware); | ||
}); | ||
|
||
adapter.onTurnError = async (context, err) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a nit of mine, but this could more succinctly be written as adapter.onTurnError = (context, err) => context.sendActivity(err.message)
. No need to explicitly mark as async or await the result.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thx, I tried this but the return type of onTurnError
is Promise<void>
and the return type of context.sendActivity(err.message)
is Promise<ResourceResponse>
, how can I fix the mismatch issue?
|
||
for (let i = 0; i < this.script.length; i++) { | ||
const testAction = this.script[i]; | ||
await testAction.execute(testAdapter, bot.onTurn.bind(bot)); | ||
await testAction.execute(adapter, callback); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd rewrite this block to:
await Promise.all(this.script.map(testAction => testAction.execute(adapter, callback)));
I wonder if it would also be useful to use something like p-map
and a configurable concurrency
to control concurrent execution. What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a way to execute all the scripts in order?
Fixes #2959
Description
Refactored TestRunner.
Specific Changes
TestRunner
toTestUtils
to match naming in C#TestUtils
runTestScript
staticrunTestScript
Testing
Updated existing tests.