Clean up stored procedure tests as NonSharedModelTestBase #29331
Conversation
roji
force-pushed
the
SprocTestCleanup
branch
from
a8a3c17
to
7ea6151
Compare
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
@danmoseley |
test/EFCore.Relational.Specification.Tests/Update/StoredProcedureUpdateTestBase.cs
Outdated
Show resolved
Hide resolved
roji
force-pushed
the
SprocTestCleanup
branch
from
7ea6151
to
936bbc0
Compare
|
Hello @roji! 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 (
|
| protected virtual void CreateStoredProcedures(DbContext context, string createSprocSql) | ||
| { | ||
| foreach (var batch in | ||
| new Regex("^GO", RegexOptions.IgnoreCase | RegexOptions.Multiline, TimeSpan.FromMilliseconds(1000.0)) |
There was a problem hiding this comment.
Feel free to ignore, but regex timeouts are a protection against DOS attacks - I assume input here is trusted. Given that test machines are occasionally perversely slow, I wonder whether you want a timeout here, or if a timeout then a large one like 1 min.
There was a problem hiding this comment.
Yeah, this is test-only code which we already use elsewhere, and haven't ever seen timeouts AFAIK... We can definitely crank it up if this happens (or just not use regex for this trivial thing).
|
Apologies, while this PR appears ready to be merged, I've been configured to only merge when all checks have explicitly passed. The following integrations have not reported any progress on their checks and are blocking auto-merge:
These integrations are possibly never going to report a check, and unblocking auto-merge likely requires a human being to update my configuration to exempt these integrations from requiring a passing check. Give feedback on thisFrom the bot dev teamWe've tried to tune the bot such that it posts a comment like this only when auto-merge is blocked for exceptional, non-intuitive reasons. When the bot's auto-merge capability is properly configured, auto-merge should operate as you would intuitively expect and you should not see any spurious comments. Please reach out to us at fabricbotservices@microsoft.com to provide feedback if you believe you're seeing this comment appear spuriously. Please note that we usually are unable to update your bot configuration on your team's behalf, but we're happy to help you identify your bot admin. |
|
Apologies, while this PR appears ready to be merged, it looks like |
This is a test-only change which redoes the sproc tests as NonSharedModelTestBase, making them much less brittle and encapsulating each tests requirements inside the test. Since these tests are new, I'd like to get this in for 7.0 if possible, so that external providers can use this new model and not have to do another change later.