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

add bigint detection and sets mssql req.input when appropriate #1445

Merged
merged 5 commits into from May 29, 2016

Conversation

Projects
None yet
2 participants
@sandorfr
Contributor

sandorfr commented May 24, 2016

@elhigu

This comment has been minimized.

Collaborator

elhigu commented May 24, 2016

Implementation looks correct, but this really would need test case too.

// sets a request input parameter. Detects bigints and sets type appropriately.
_setReqInput(req, i, binding) {
if (typeof binding == 'number' && (binding < -2147483648 || binding > 2147483647)) {
if (binding < -9007199254740991 || binding > 9007199254740991) {

This comment has been minimized.

@elhigu

elhigu May 24, 2016

Collaborator

Would be nice for the code to give names for the magic numbers used e.g. MIN/MAX_INT4 MIN/MAX_SAFE_INTEGER. Anyways we are using babel to compile sources to ES5 so I suppose Number.MAX_SAFE_INTEGER / Number.isSafeInteger() should work too.

This comment has been minimized.

@sandorfr

sandorfr May 24, 2016

Contributor

Ok for the constants, isSafeInteger wasn't transpiled or polyfiled (I guess this is a matter of babel profile and plugin) Maybe constants would be the most compatible approach?

This comment has been minimized.

@elhigu

elhigu May 25, 2016

Collaborator

Yep, sounds good.

This comment has been minimized.

@sandorfr

sandorfr May 25, 2016

Contributor

OK will do soon and add some unit tests.

@sandorfr

This comment has been minimized.

Contributor

sandorfr commented May 29, 2016

@elhigu I somewhat failing at writing units test for that cas and I can't find one testing similar issue. I tried to mock the connection but it is still trying to connect to a real database :( ). I you can give me a hint :)

@elhigu

This comment has been minimized.

Collaborator

elhigu commented May 29, 2016

I have no I idea if mocking the client is even possible, do you have mssql installed so that you could just write integration test for it?

@sandorfr

This comment has been minimized.

Contributor

sandorfr commented May 29, 2016

@elhigu ok just pushed them. I have a questions :). Why bail option is activated by default on test script (this bugged me a while)? And I see 24 tests failing for mssql, they don't appear to be any related to my changes so I guess mssql support is still a WIP?

@elhigu

This comment has been minimized.

Collaborator

elhigu commented May 29, 2016

It is really hard to keep track how mssql / oracle are working since I have never ran mssql tests myself... Probably at some point all tests were passing and I suppose they are broken again. You should try running tests before applying your changes to check if errors are caused by your changes...

I just few hours ago sent email to microsoft people if they could provide me preview of their upcoming mssql for linux version and if there is any possibility to set it up for knex CI... not having high hopes for that though :) maybe some day.

Test bailing is on at least because of integration tests are stateful so if one test fails, it is possible that all the tests will fail after that since DB is not in correct state. Though I don't know what has been real motivation for that.

@elhigu

This comment has been minimized.

Collaborator

elhigu commented May 29, 2016

@sandorfr could you still confirm that other test failures were not caused by this change and maybe add an issue with errors of failing tests if mssql is currently broken?

@sandorfr

This comment has been minimized.

Contributor

sandorfr commented May 29, 2016

Just tested, the errors are on master branch, I think some of them are just outdated tests. I'll open an issue for that.

Regarding mssql tests a basic azure instance is just 5 euros per month so I can provide you an access to one until Microsoft releases mssql for linux (or invites you to join the beta).

@elhigu elhigu merged commit 05a95c2 into tgriesser:master May 29, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@elhigu

This comment has been minimized.

Collaborator

elhigu commented May 29, 2016

@sandorfr thanks for the offer I'll discuss about using azure mssql instance for CI with other collaborators 👍

@elhigu

This comment has been minimized.

Collaborator

elhigu commented May 31, 2016

@sandorfr now that I was thinking how to use external DB from travis CI so that there can be multiple tester instances running against the same DB at the same time, would require some extra coding to implement. If someone is able to make it work I would accept pullrequest for that though :)

@sandorfr

This comment has been minimized.

Contributor

sandorfr commented May 31, 2016

@elhigu Yes concurrency is definitely non trivial. I think one approach would be to use a different schemas for each build (and drop the schema and its objects at the end of the test run). But I think the current implementation for mssql does not support schemas and assumes it is dbo (see https://github.com/tgriesser/knex/blob/master/src/dialects/mssql/query/compiler.js#L163 for instance). So the first step would be to enable schema support for mssql.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment