-
Notifications
You must be signed in to change notification settings - Fork 31
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 for #127 Is not properly using TaskHubName #128
Fix for #127 Is not properly using TaskHubName #128
Conversation
…sing default TestHubName or DurableTaskOption HubName instead of using the specified TaskHub
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.
Thanks for this PR! I few questions/comments:
test/DurableTask.SqlServer.AzureFunctions.Tests/IntegrationTestBase.cs
Outdated
Show resolved
Hide resolved
test/DurableTask.SqlServer.AzureFunctions.Tests/CoreScenarios.cs
Outdated
Show resolved
Hide resolved
src/DurableTask.SqlServer.AzureFunctions/SqlDurabilityProviderFactory.cs
Outdated
Show resolved
Hide resolved
Hello @cgillum I reworked a bit the test so that we also use custom schema. |
test/DurableTask.SqlServer.AzureFunctions.Tests/MultiTenancyCoreScenarios.cs
Outdated
Show resolved
Hide resolved
test/DurableTask.SqlServer.AzureFunctions.Tests/MultiTenancyCoreScenarios.cs
Outdated
Show resolved
Hide resolved
…no mining in multi tenancy case.
Hello @cgillum any news on 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.
Thanks for the ping, and so sorry for dropping the ball on this PR. My only remaining ask is if we can drop a few things that I don't think are fully necessary. After that, I'm good to accept this.
test/DurableTask.SqlServer.AzureFunctions.Tests/MultiTenancyCoreScenarios.cs
Outdated
Show resolved
Hide resolved
All good for me @cgillum |
@cgillum I fixed the test it was timeout due to purge but it's not required in multitenancy tests |
It looks like there might be a test concurrency issue now that we have multiple integration test classes . Here's the error from the CI:
The solution mentioned here might be good to look into. |
Thx for the tips @cgillum was looking at it. could you rerun I hope attribute on CoreScenario is going to work with inheritance |
Great |
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.
Alright, everything looks good. Thank you for this contribution!
Fixed #127 Durability factory when calling for a client is using default TestHubName or DurableTaskOption HubName instead of using the specified TaskHub