Adding tests for mssql - Githubissues

dougmolineux commented 4 years ago

Greetings!

I have been working on adding tests to mock-knex for mssql, since I have been using this a lot in a project of mine, and its so useful :). I wanted to initially add a single test (due to https://github.com/colonyamerican/mock-knex/issues/91) however I noticed that there weren't any mssql tests in here. This is my attempt at getting the base test-suite to work with mssql.

I had to make a few modifications to get the make test-suite command to work with the new mssql directory. I'll attempt to describe the modifications here.

docker-compose.yml: Modified to setup a MSSQL database (running on a different port than usual, due to port conflict with one of the other dbs)

package.json: Added the node mssql official module

test directory: I copied over one of the existing folders (mysql), renamed it to be "mssql" and then made some modifications within there. I had some trouble getting this test working in particular, titled "should support schema#hasTable". I was a bit fuzzy on the differences between information_schema.tables and sys.tables so I added what the mssql implementation was setting the query to be in the expect array. This maybe incorrect, so please let me know if I'm doing something wrong here.

platform directory: After adding the mssql tests, for some reason, the Transaction tests required the mock connection to have new transaction functions (begin, rollback and commit). So I have added them to get rid of the errors I was seeing. This might be a misunderstanding on my part on how this is working, so I welcome feedback.

test/run.js: I had to modify this because some of the older versions of knex didn't even have the dialects/mssql folder at all. I believe that knex didn't add support for mssql until version 0.11, so I needed to selectively only run the mssql tests when a valid version of knex was installed. In particular, version 0.15 also had issues with the version of the mssql module I am using.

Please feel free to provide any feedback, I would be happy to make any changes to this. Also, if it isn't helpful at all, thats ok too,

Thank you!

jbrumwell commented 4 years ago

@dougmolineux Great work, thank you for your contribution :+1: One concern I have is that the connection change seems to be specific to mssql and by stubbing connections across all db variants we may run into a conflict in the future.

This is something we could address later on, also with 0.15 do the tests pass with the mssql version that knex suggests?

dougmolineux commented 4 years ago

Thanks for the response! Hmm, yes, I understand the concern about adding "transaction" to all the mockedConnection. I will need to dig in here and see if we can only add these functions when testing the mssql stuff.

Good question about the 0.15, I'm not sure but I will try the latest version, and see if it works. If so, I will remove that version from the "mssql unsupported" array.

jbrumwell commented 4 years ago

@dougmolineux I have some thoughts around the connection as possible work arounds;

We could define the connection object as a Proxy and pass it the dialect being used. We could then conditionally allow different api's for each dialect as needed
Another option would be to alter the define transform to allow passing in a object with transforms based on dialect.

I can probably look at the first one tonight and see if it works. If you could look at trying to get mssql tests running for each version knex supports that would be very helpful :)

dougmolineux commented 4 years ago

Here's error I receive when knex is version 0.15 and mssql is 5.1.0 (which is the latest version: https://www.npmjs.com/package/mssql)

Error: This knex version does not support any other mssql version except 4.1.0 (knex patches bug in its implementation)

However, when I lower the version of mssql down to 4.1.0, all the tests work. It's not really ideal since its the not the newest version of mssql, but it appears this version is more compatible with knex, so I have pushed an update to my PR here. Please let me know if you think I should revert my changes, use 5.1.0 and add 0.15 back to the unsupported version.

Thank you!

jbrumwell commented 4 years ago

@dougmolineux proxy I don't think is any better as we can't acquire the dialect that is being used from what I can see. I'm okay with merging this can you check again that it works as expected for you?

jbrumwell / mock-knex

Adding tests for mssql #98