Test only against SQLServer 2016. #420

arthurschreiber · 2016-08-08T10:19:08Z

This changes the appveyor tests to run only against SQL Server 2016. But it also enables separate testing of all protocol versions.

arthurschreiber · 2016-09-08T21:45:04Z

@tvrprasad These changes here allow running the tedious integration tests using a specific version of the TDS protocol, instead of the latest version supported by the sql server that is used for the tests.

The version can be specified using the TEDIOUS_TDS_VERSION environment variable. IIRC, older versions of the TDS protocol (7_3_A, 7_2 and 7_1) are completely unusable with tedious right now. I'd be super grateful if you could take a look and maybe figure out what's going on there?

We've had a few users complaining that they ran into issues when connecting to older SQL Server versions, getting the failing integration tests working again would be a great first step here. 👍

tvrprasad · 2016-09-14T17:59:58Z

@arthurschreiber I'll take a look at this in the next few days or so.

Sent you a couple of PRs on other issues. If you're able to merge them (or give feedback), that'll be great. Thanks.

returns a null value for Variant type. This change addresses that.

- DateTime2 is only supported in 7_3_A and later. - VarChar max length is 8000 before 7_2. - NVarChar max length is 4000 before 7_2.

Tds multi version

events 'beginTransaction', 'commitTransaction' and 'rollbackTransaction'. These are all available only for TDS versions 7.2 and over. This is causing several transaction integration tests to fail. This PR attempts to fill in the gaps by managing inTransaction state based on equivalent events and callbacks for TDS versions below 7.2. All the tests pass now except for transactionHelperMixedWithLowLevelTransactionMethods.

Update transactionDepth for Savepoint in transaction(). With this all the tests pass for all versions TDS.

Change callback invocations for TDS version < 7.2 in beingTransaction, commitTransaction, rollbackTransaction and saveTransaction to not expose the internal Request object instance created to send the TRANSACTION SQL commands.

…7.2.

Currently inTransaction state on Connection is maintained based on the

tvrprasad · 2016-10-06T16:29:11Z

Just saw the test failures. Seems like ...args syntax is not supported in NodeJS 4.5 and NodeJS 5.12 but supported in older and newer versions. That's odd. I'm looking.

tvrprasad · 2016-10-06T17:26:31Z

The rest parameters (i.e. the ...args) syntax is indeed not support in older versions of NodeJS. See http://node.green/ for the support status for different versions.

The reason tests are passing for 0.12 and 0.10 is because of this magic line in test/setup.js:

if (require('semver').lt(process.version, '4.0.0')) {
  require('babel-register');
}

We have two options:

Change 4.0.0 to 6.0.0 in the above snippet.
Revert back to function() syntax from => syntax and rely on self=this trick.

I don't quite understand the implications of 1) at the moment. Please me know which option you like better and I'll send a PR. Thanks.

arthurschreiber · 2016-10-06T17:33:49Z

I'm on a plane right now, I'll write up a longer response with all the
details later.

For now, please revert to the 'function' syntax for the cases where you
need the arguments object. Thanks. ❤️
tvrprasad notifications@github.com schrieb am Do. 6. Okt. 2016 um 19:26:

The rest parameters (i.e. the ...args) syntax is indeed not support in
older versions of NodeJS. See http://node.green/ for the support status
for different versions.

The reason tests are passing for 0.12 and 0.10 is because of this magic
line in test/setup.js:

if (require('semver').lt(process.version, '4.0.0')) {
require('babel-register');
}

We have two options:

Change 4.0.0 to 6.0.0 in the above snippet.

Revert back to function() syntax from => syntax and rely on self=this
trick.

I don't quite understand the implications of 1) at the moment. Please me
know which option you like better and I'll send a PR. Thanks.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#420 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAAIk9jk8FW0UXwwSYw2_sewDLKAhadEks5qxS9IgaJpZM4Je5pJ
.

…backs." Rest parameters syntax (i.e. the ...args syntax) is not supported in older versions of NodeJS. Hence the revert. This reverts commit cd7f4b6.

Revert "Switch from function() style to => in transaction method call…

tvrprasad · 2016-10-06T19:08:54Z

Responding from a plane, wow! Very much appreciate it :)
Reverted the function() to => change. Tests passing now.

arthurschreiber · 2016-10-06T23:01:27Z

Thanks, this is great! Gonna merge this now. ❤️

arthurschreiber · 2016-10-06T23:14:38Z

@tvrprasad We use a library called babel to convert the ES6/ES2016 source code of tedious to ES5 when releasing a new version. This allows us to write "modern" JavaScript while supporting older Node versions like 0.10 and 0.12.

The lines you pointed out in the test/setup.js was intended to make sure we don't load babel when running the tests on node versions newer or equal to the current LTS version (that is, at the time of writing, 4.x). The idea here was to make sure we only use modern JS features that are supported in the current Node LTS.

I'm not 100% sure if that's the best approach - maybe we should simply embrace ES2016 as a whole?

tvrprasad · 2016-10-07T16:31:07Z

Thanks for the explanation. That makes sense now.

I'm not 100% sure if that's the best approach - maybe we should simply embrace ES2016 as a whole?

What in your mind is the value proposition in limiting which modern JS features we use?

The only thing I can think of is that using current Node LTS supported features would make it easier for developers to contribute to the project.

In build.coffee where we run the babel compiler, we're running it for all versions of node. Why is that? I'm guessing it's so we don't introduce modern syntax issues we didn't catch in tests but not sure.

tvrprasad · 2016-10-07T17:29:14Z

One other topic. How do we manage dropping support for older versions of node.js? In fact v0.10 just fell out of maintenance this month per https://nodesource.com/blog/essential-steps-long-term-support-for-nodejs/. Do we stop supporting it for new releases of Tedious? Perhaps this is only of academic interest at this point but could be real if we start run into some issue that's only on v0.10.

arthurschreiber · 2016-10-07T20:03:03Z

One other topic. How do we manage dropping support for older versions of node.js? In fact v0.10 just fell out of maintenance this month per https://nodesource.com/blog/essential-steps-long-term-support-for-nodejs/. Do we stop supporting it for new releases of Tedious? Perhaps this is only of academic interest at this point but could be real if we start run into some issue that's only on v0.10.

I'd be in favour of dropping support for v0.10 and v0.12 (plus all iojs versions), not with the next release, but the one after. Do you think that makes sense?

arthurschreiber · 2016-10-07T20:03:58Z

In build.coffee where we run the babel compiler, we're running it for all versions of node. Why is that? I'm guessing it's so we don't introduce modern syntax issues we didn't catch in tests but not sure.

This happens so we can push a single package to npm, that is used across all supported versions of Node.js.

tvrprasad · 2016-10-07T22:07:16Z

I'd be in favour of dropping support for v0.10 and v0.12 (plus all iojs versions), not with the next release, but the one after. Do you think that makes sense?

That sounds reasonable to me.

This happens so we can push a single package to npm, that is used across all supported versions of Node.js.

Ah. That makes perfect sense!

arthurschreiber added 3 commits August 8, 2016 12:18

Test only against SQLServer 2016.

d701813

This changes the appveyor tests to run only against SQL Server 2016. But it also enables separate testing of all protocol versions.

Add missing semicolon.

4a42be4

Fix appveyor config.

b7280e7

arthurschreiber mentioned this pull request Sep 29, 2016

Tedious releases #455

Closed

tvrprasad added 2 commits October 1, 2016 17:38

The code currently does not handle correctly the case when the server

4a0b5da

returns a null value for Variant type. This change addresses that.

Fix test issues with older versions of TDS protocol:

5c39926

- DateTime2 is only supported in 7_3_A and later. - VarChar max length is 8000 before 7_2. - NVarChar max length is 4000 before 7_2.

tvrprasad mentioned this pull request Oct 2, 2016

Tds multi version #460

Merged

arthurschreiber and others added 2 commits October 2, 2016 12:03

Merge pull request #460 from tvrprasad/tds-multi-version

527f74f

Tds multi version

tvrprasad mentioned this pull request Oct 3, 2016

Currently inTransaction state on Connection is maintained based on the #461

Merged

tvrprasad added 7 commits October 3, 2016 10:15

Fix test failure transactionHelperMixedWithLowLevelTransactionMethods.

8ada2df

Update transactionDepth for Savepoint in transaction(). With this all the tests pass for all versions TDS.

Fix typo. Change this -> self in one place in Connection.transaction().

38ecbd2

Change == to === for SQL_BATCH comparison.

4c6916b

Set transactionDepth and isSqlBatch properties only for tdsVersion < …

1702a3e

…7.2.

Switch from function() style to => in transaction method callbacks.

cd7f4b6

Merge pull request #461 from tvrprasad/tds-multi-version

b7fd6cc

Currently inTransaction state on Connection is maintained based on the

Revert "Switch from function() style to => in transaction method call…

0e4e5f4

…backs." Rest parameters syntax (i.e. the ...args syntax) is not supported in older versions of NodeJS. Hence the revert. This reverts commit cd7f4b6.

tvrprasad mentioned this pull request Oct 6, 2016

Revert "Switch from function() style to => in transaction method call… #465

Merged

Merge pull request #465 from tvrprasad/arrow-to-function

505b4d8

Revert "Switch from function() style to => in transaction method call…

arthurschreiber merged commit aa57d3c into master Oct 6, 2016

arthurschreiber deleted the arthur/faster-appveyor-tests branch November 12, 2016 15:00

LuisBosquez mentioned this pull request Jan 13, 2017

Dead, or merely dormant? Azure/node-sqlserver#189

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Test only against SQLServer 2016. #420

Test only against SQLServer 2016. #420

arthurschreiber commented Aug 8, 2016

arthurschreiber commented Sep 8, 2016

tvrprasad commented Sep 14, 2016

tvrprasad commented Oct 6, 2016

tvrprasad commented Oct 6, 2016

arthurschreiber commented Oct 6, 2016

tvrprasad commented Oct 6, 2016

arthurschreiber commented Oct 6, 2016

arthurschreiber commented Oct 6, 2016

tvrprasad commented Oct 7, 2016

tvrprasad commented Oct 7, 2016

arthurschreiber commented Oct 7, 2016

arthurschreiber commented Oct 7, 2016

tvrprasad commented Oct 7, 2016

Test only against SQLServer 2016. #420

Test only against SQLServer 2016. #420

Conversation

arthurschreiber commented Aug 8, 2016

arthurschreiber commented Sep 8, 2016

tvrprasad commented Sep 14, 2016

tvrprasad commented Oct 6, 2016

tvrprasad commented Oct 6, 2016

arthurschreiber commented Oct 6, 2016

tvrprasad commented Oct 6, 2016

arthurschreiber commented Oct 6, 2016

arthurschreiber commented Oct 6, 2016

tvrprasad commented Oct 7, 2016

tvrprasad commented Oct 7, 2016

arthurschreiber commented Oct 7, 2016

arthurschreiber commented Oct 7, 2016

tvrprasad commented Oct 7, 2016