-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
[Service Bus] Update perf tests to use a single sender for all instances #14460
Conversation
HarshaNalluru
commented
Mar 24, 2021
- Imports v7 in the v1 tests to allow creating the resources
- Makes sender static in both v1 and v7
@@ -8,6 +8,7 @@ | |||
"license": "ISC", | |||
"dependencies": { | |||
"@azure/service-bus": "^1.1.10", | |||
"@azure/service-bus-v7": "npm:@azure/service-bus@^7.0.3", |
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.
Wow, did not know you could do this!
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.
Learned it yesterday! :)
All hail npm! All hail SOF!!!
|
||
export abstract class ServiceBusTest<TOptions> extends PerfStressTest<TOptions> { | ||
sbClient: ServiceBusClient; | ||
static sbClient: ServiceBusClient = new ServiceBusClient(connectionString); |
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.
Any reason not to do this in globalSetup()?
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.
Doesn't matter. It would work the same way.
In my mind.. I've kept globalSteup for creating the resources and the initialization of clients at the static variable declaration itself.
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.
Looks good. Had a small comment that's mostly just consistency, not functionally affecting that I can see.
@HarshaNalluru Can this be merged? |
Hello @HarshaNalluru! Because this pull request has the p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (
|
…ces (Azure#14460) - Imports v7 in the v1 tests to allow creating the resources - Makes sender static in both v1 and v7