ref #2597 Fix function getListTablesSQL() for SQL Server #2598
Conversation
|
Needs tests, both unit and functional ones |
|
@Ocramius : i'm a newbee on posting fix on github, what's needed exactly ? |
|
See https://github.com/doctrine/dbal/blob/master/tests/Doctrine/Tests/DBAL/Platforms/SQLServerPlatformTest.php for an example of how existing tests for this platform were written. Also, a test that fails before this patch is applied, and describes what the original issue is, is needed. We generally do not merge work without full coverage of an issue. |
|
@Ocramius : If i'm writing tests for this patch, do i need to create a full new PR ? |
|
No, just keep pushing commits to the branch you were working on, and then
simply poke us when you want us to check it 👍
…On 12 Jan 2017 15:56, "cgardiennet" ***@***.***> wrote:
@Ocramius <https://github.com/Ocramius> : If i'm writing tests for this
patch, do i need to create a full new PR ?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#2598 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AAJakAsaFadVBFOHmQ4218MjROSeCeioks5rRj8TgaJpZM4Lh0OH>
.
|
|
@cgardiennet have you had a chance to get to the unit tests for this? Seems like a good improvement just needs to get tests implemented. |
|
Poking the thread as it's been a year already. I applied the change on my project and it fixed the related issues with the schemas and SQL Server. I am not currently able to put tests in place though. |
|
@malandles Unfortunately tests are still missing and we can't consider bugfixes without them. If you can provide one, it could help resolving this bug/PR. |
|
I'll try to provide tests, but I didn't have time for this PR. And I also don't have an SQL Server DB available for now. |
|
@cgardiennet Hi, we've added CI testing (Travis + AppVeyor) against multiple SQL Server versions recently in #2617 & #3050. You can now try writing a blind functional test and see how it goes on CI without having SQL Server locally. 😏 |
|
Closing as stale and missing tests. |
Fix done for ref #2597!
Waiting for review and merge !