Enable use of docker in travis and run tedious + stress tests in CI by elhigu · Pull Request #614 · tediousjs/node-mssql

elhigu · 2018-02-26T11:28:10Z

This should also include fixes to current test problems in test-tedious when using .mssql-docker.yml configuration.

Written on top of PR that fixes pool create issue in rare connection failure cases.

This also includes fix for the problem #612 and added test to CI that connecting doesn't hang anymore.

elhigu · 2018-02-26T15:13:46Z

I'm having some failure problems with these tests... stay tuned, I already did put so much time to this that I can't stop now 😅

elhigu · 2018-03-04T20:58:03Z

Now that implementation is using end event and npm run mssql-server:up is actually waiting until mssql server is ready all seems to run correctly.

Commit history is a bit ugly though so squash merge would be probably good choice to use if this is chosen to be merged in. Meanwhile I see if I'm able to write workaround for this to knex.

elhigu · 2018-03-04T23:46:18Z

+          reject(err)
+        }
+      }
+


Below this line is the actual fix, that we are rejecting also if end is emitted, even that connectnever triggered.

Maybe explaining what is happening and why in a comment in the code would make sense?

Yeah, better to add some information to comments too 👍

kibertoad · 2018-04-05T22:15:40Z

+  })
+}
+
+function cloneDeep (val) {


since this is reused, maybe makes sense to extract to test util file? or import lodash as devdependency and use _.cloneDeep?

I didn't want to add even extra dev dependency for simple one liner, I'll refactor this and delay functions to test utils.

kibertoad · 2018-04-05T22:16:27Z

+    debug('========== Reconnecting mssql')
+
+    // this test needs toxiproxy to run so docker mssql is also required
+    mssqlCon = new mssql.ConnectionPool({


does the config belong inside the test itself?

Yes, this time configuration should be hardcoded, because it must match the docker-compose setup which is hardcoded as well (but in yaml language instead of js / json).

kibertoad · 2018-04-05T22:16:48Z

+  console.log('------------------------ REQUESTS SINCE LAST PRINT ------------------------')
+  if (mssqlCon.pool) {
+    console.dir({
+      peding: mssqlCon.pool.pending,


peding -> pending

kibertoad · 2018-04-05T22:17:27Z

+
+  if (hangRounds === 3) {
+    console.log('Driver did hang exit with error')
+    process.exit(1)


since this is a test, does it make sense to exit process and not just fail an assertion?

Probably throwing exception and leaking it out kill the process would work too, but I think that calling exit(1) reflects more explicitly about that it is really intended to close this this process if hang is detected.

kibertoad · 2018-04-05T22:18:14Z

+    })
+  } catch (err) {}
+
+  const proxy = await toxicli.createProxy({


maybe all the proxy code would make more sense in a separate helper file to reduce the clutter?..

It is already in helper called recreateProxy, I find it better to have that code easily accessible than refactored prematurely.

Also the kind of proxy setup one needs to create depends on code that is being tested (I already wrote this kind of code for multiple DB drivers), so general helper doesn't make sense yet (unless there will be more tests like this using the same proxycreation).

elhigu · 2018-04-06T06:31:44Z

@kibertoad Thanks for the review, I'll be updating this as soon as I have some spare time.

willmorgan · 2018-10-18T20:56:18Z

I think in the short term while we're still figuring out what to do with AppVeyor (assuming stress tests are run there), then it'll give us something controlled to test on.

Also included default config, which will use mssql running in docker for testing

elhigu · 2018-10-18T21:20:23Z

Adding that stresstest sounds fine if its added as a separate travis instance doesnt sound like a bad thing if it works on travis fine. Also I think that this PR doesn’t contain all the canges I made to knex side so some cross referencing from there could be done. Also some of the stuff in this PR might have already been fixed in current master.

…nched with 'end' event

willmorgan · 2018-10-18T21:35:11Z

+try {
+  config = require(`./.mssql.json`)
+} catch (e) {
+  config = require(`./.mssql-docker.json`)


what would be the expected error here?

file doesn't exist?

shouldn't we just include the correct file depending on the context? 😱

🤷‍♂️

.mssql-docker.json docker file is included in repo with correct settings for dockerized mssql and if someone likes to use he's own mssql installation he can just add that .mssql.json which is not added to repo to override usage of docker... I don't have much of preference how it is signaled which configuration needs to be used. This was the easiest choice to implement and maintain that I came up :) Arguable maybe not the most elegant, but I like promoting functional design over elegancy.

I'm really not bothered, the only thing I may have changed is to call it .mssql,dist.json

The configuration file is tightly coupled with the configuration in docker file, that is why I choose the name also to refer to docker. Though .dist.json suffix could be good in either way 👍

willmorgan · 2018-10-18T21:37:53Z

+
 const config = function () {
-  let cfg = JSON.parse(require('fs').readFileSync(`${__dirname}/../.mssql.json`))
+  let cfg = clone(require('../mssql-config'))


I'd be in favour of assigning the mssql-config require once at the top (eslint global-require style) and then cloning that assignment like so:

const configOptions = require('../mssql-config'); // later... function config() { return Object.assign({ driver: 'tedious' }, configOptions); }

PRS WELCOME

yup, that would be more readable

dhensby

Quite a few of these commits can be squashed too, to neated it up

dhensby · 2018-10-18T21:33:56Z

-    "test-cli": "node_modules/.bin/mocha test/common/cli.js"
+    "test-cli": "node_modules/.bin/mocha test/common/cli.js",
+    "mssql-server:up": "docker-compose up --no-start &&  docker-compose start",
+    "mssql-server:down": "docker-compose down"


should this be docker-compose down -v?

dhensby · 2018-10-18T21:36:44Z

      debug('pool(%d): connection #%d created', IDS.get(this), IDS.get(tedious))
      debug('connection(%d): establishing', IDS.get(tedious))

+      tedious.on('debug', msg => {


is this needed?

It was, because some disconnect events were not coming through, but I think this is one thing that in knex side was implemented in a different way in the patch I made there.

So... probably this is not needed, but something else was.

Yeah, to fix the pool depletion bug you needed the safeReject and safeResolve methods and to attach to the end event as well as the error because end was being fired by tedious (instead of error) if there was a socket timeout during connections.

That's not the case anymore so I've completely removed that patch, so this can go as well, I think.

willmorgan · 2018-10-18T23:12:39Z

@dhensby Closing this PR should be fairly high priority as it's blocking this old chestnut from getting removed:

https://github.com/tgriesser/knex/blob/232fe9f1517dba927f6a3a1fb1b8842d7c0a4007/src/dialects/mssql/index.js#L52-L70

cc: @elhigu

dhensby · 2018-10-19T06:36:44Z

@willmorgan this PR is not blocking that. This is: tediousjs/tedious#809

See knex/knex#2861

dhensby · 2018-10-19T09:09:21Z

Feedback addressed

willmorgan · 2018-10-19T09:36:55Z

@willmorgan this PR is not blocking that.

I think it actually is - see the comment in the highlighting code that refers back to this very PR.

dhensby · 2018-10-19T12:21:07Z

I think it actually is - see the comment in the highlighting code that refers back to this very PR.

I'm literally working on all 3 aspects, this PR is not blocking anything. This PR has gone from fixing the pool depletion bug to simply providing a resilient test suite. The fix for pool depletion has been removed from this PR and as the error event is correctly emitted on aborted connections now.

dhensby · 2018-10-20T10:12:00Z

Right; I'm a little concerned that the stress test is not failing (because it should be due to the issue fixed with tediousjs/tedious#809 (not yet merged / released).. The stress tests in the knex library do fail without the current fix....

dhensby · 2018-10-20T20:05:17Z

OK - so something that's not so clear to me and @elhigu I'd like your thoughts on this:

I'd expect that this stress test would fail the same way the knex one would.

If I check out this branch of knex (to allow running non-4.1.0 versions of mssql) https://github.com/dhensby/knex/tree/fix/mssql-support and then install this PR's branch of mssql and run the stress test there, I get the error noted here tediousjs/tedious#809.

But if I run the stress test in this PR I get no error (even when I allow the test to run indefinitely). Therefore there must be some material difference to how this stress test is implemented.

dhensby · 2018-11-19T11:51:11Z

I'm going to have to close this due to lack of feedback and the fact it's not actually replicating the problems that other stress tests do. I'll happily re-open once we've got an update

kibertoad · 2018-11-19T14:41:52Z

@dhensby Wait, aren't stress tests passing because this PR also includes fixes, as mentioned by @elhigu in pr description?

dhensby · 2018-11-19T14:54:31Z

@kibertoad nope, I pulled the fixes out into their own PR: #719 which wasn't merged in the end because the bug was fixed in the upstream library instead.

But when I run the stress test that comes with knex I then got an error (which was fixed with tediousjs/tedious#809) but that error does not happen with this stress test, so there must be some difference

elhigu force-pushed the run-ci-tests-against-mssql-server branch 14 times, most recently from f4c52fc to 840f585 Compare February 26, 2018 14:16

elhigu force-pushed the run-ci-tests-against-mssql-server branch 4 times, most recently from ddca31b to 3efd83e Compare March 4, 2018 20:53

This was referenced Mar 4, 2018

Detect closed tedious client during pool create #612

Closed

Still seeing generic-pool errors in 0.14.4 knex/knex#2522

Closed

Is this project maintained? #601

Closed

elhigu commented Mar 4, 2018

View reviewed changes

kibertoad reviewed Apr 5, 2018

View reviewed changes

kibertoad mentioned this pull request Oct 18, 2018

Run ci tests against mssql server (v2) #721

Closed

elhigu added 5 commits October 18, 2018 22:17

Added docker compose file to run mssql server on any platform

d93686d

Also included default config, which will use mssql running in docker for testing

Added stress test for flaky connections

4c0cae9

Workaround for detecting closed tedious connection during poolCreate

61622bd

Fixed styling issues

d8e81bb

Enable running tests against mssql server in CI

d4fb0ae

elhigu added 3 commits October 18, 2018 22:22

Fixed tedious tests with .mssql-docker.json config

fda7eb2

Fixed detecting closed connection before 'connect' event has been lau…

1570d6d

…nched with 'end' event

Fixed docker mssql up script to wait for DB is running

00b2dbc

dhensby force-pushed the run-ci-tests-against-mssql-server branch from 3efd83e to 00b2dbc Compare October 18, 2018 21:25

Update package-lock

103aae7

willmorgan reviewed Oct 18, 2018

View reviewed changes

dhensby reviewed Oct 18, 2018

View reviewed changes

dhensby added 2 commits October 19, 2018 09:57

Move docker init script to test/scripts

6722084

remove reject during debug

35a7876

Nice config cloning

69869af

dhensby force-pushed the run-ci-tests-against-mssql-server branch from a505ec0 to 69869af Compare October 19, 2018 09:11

dhensby closed this Nov 19, 2018

Uh oh!

Conversation

elhigu commented Feb 26, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

elhigu commented Feb 26, 2018

Uh oh!

elhigu commented Mar 4, 2018

Uh oh!

elhigu Mar 4, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

elhigu commented Apr 6, 2018

Uh oh!

willmorgan commented Oct 18, 2018

Uh oh!

elhigu commented Oct 18, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dhensby left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

willmorgan commented Oct 18, 2018

Uh oh!

dhensby commented Oct 19, 2018

Uh oh!

elhigu commented Feb 26, 2018 •

edited

Loading

elhigu Mar 4, 2018 •

edited

Loading