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
Added FakeScaleoutBus to functional tests #2097
Conversation
If you don't pass 3 arguments using InlineData what happens? It would be good if we can avoid passing it, but if not it looks fine. |
The default MessageBus will be used if the FakeScaleoutBus isn't specified in ITestHost.Initialize so you don't need to pass MessageBusType as an InlineData argument if you just want to use the default bus. In fact, I already leave this argument out for a few tests that are IISExpress only. |
{ | ||
var message = new ScaleoutMessage(messages); | ||
|
||
OnReceived(streamIndex, _id, message); |
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.
We may want to trigger OnReceived asynchronously to more accurately represent a real scenario.
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.
ThreadPool.UnsafeQueueUserWorkItem? What do you think @davidfowl?
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.
No this is fine. We just need to exercise the code path, leave it as is. Plus ThreadPool.QUWI would make this harder as you'd have to ensure there's no overlapping calls to OnReceived. What you have is good.
Should provide a way to run a test with a specific number of streams. |
int? connectionTimeout = 110, | ||
int? disconnectTimeout = 30, | ||
bool enableAutoRejoiningGroups = false, | ||
MessageBusType type = MessageBusType.Default) |
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.
Do we want to make this settable in the web.config? All other properties are for IIS. I know that IIS is only supporting the default message bus so it's not a 100% necessary change but thought I'd bring it up.
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.
We could make IISExpress tests support the FakeScaleoutBus. I'm not sure if it's worth it. It would make the tests run waaay longer.
Perhaps I should add a streams parameter back to HostedTest.UseMessageBus. I don't want to add a streams parameter to ITestHost.Initialize. That's what I had initially, but I don't what to make the Initialize signature longer than need be, or introduce the possibility for invalid configuration unless absolutely necessary. |
#2085