-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Support statement_timeout #1436
Changes from 4 commits
60dc785
41f9f7c
640495b
d5e2f33
2dcb7d6
fc8f81c
c5aec28
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4,4 +4,4 @@ build/ | |
*.log | ||
node_modules/ | ||
package-lock.json | ||
*.swp | ||
*.swp | ||
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -19,9 +19,10 @@ | |
"main": "./lib", | ||
"dependencies": { | ||
"buffer-writer": "1.0.1", | ||
"packet-reader": "0.3.1", | ||
"js-string-escape": "1.0.1", | ||
"packet-reader": "0.3.1", | ||
"pg-connection-string": "0.1.3", | ||
"pg-native": "^2.2.0", | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why this addition of pg-native ? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah shit. The tests seem to be adding that when they are run. I'll remove it. |
||
"pg-pool": "~2.0.3", | ||
"pg-types": "~1.12.1", | ||
"pgpass": "1.x", | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,62 @@ | ||
'use strict' | ||
var helper = require('./test-helper') | ||
var Client = helper.Client | ||
|
||
var suite = new helper.Suite() | ||
|
||
var conInfo = helper.config | ||
|
||
function getConInfo (override) { | ||
return Object.assign({}, conInfo, override ) | ||
} | ||
|
||
function getStatementTimeout (conf, cb) { | ||
var client = new Client(conf) | ||
client.connect(assert.success(function () { | ||
client.query('SHOW statement_timeout', assert.success(function (res) { | ||
var statementTimeout = res.rows[0].statement_timeout | ||
cb(statementTimeout) | ||
client.end() | ||
})) | ||
})) | ||
} | ||
|
||
if (!helper.args.native) { // statement_timeout is not supported with the native client | ||
suite.test('No default statement_timeout ', function (done) { | ||
getConInfo() | ||
getStatementTimeout({}, function (res) { | ||
assert.strictEqual(res, '0') // 0 = no timeout | ||
done() | ||
}) | ||
}) | ||
|
||
suite.test('statement_timeout integer is used', function (done) { | ||
var conf = getConInfo({ | ||
'statement_timeout': 3000 | ||
}) | ||
getStatementTimeout(conf, function (res) { | ||
assert.strictEqual(res, '3s') | ||
done() | ||
}) | ||
}) | ||
|
||
suite.test('statement_timeout float is used', function (done) { | ||
var conf = getConInfo({ | ||
'statement_timeout': 3000.7 | ||
}) | ||
getStatementTimeout(conf, function (res) { | ||
assert.strictEqual(res, '3s') | ||
done() | ||
}) | ||
}) | ||
|
||
suite.test('statement_timeout string is used', function (done) { | ||
var conf = getConInfo({ | ||
'statement_timeout': '3000' | ||
}) | ||
getStatementTimeout(conf, function (res) { | ||
assert.strictEqual(res, '3s') | ||
done() | ||
}) | ||
}) | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It looks like the tests only check for the correct configuration of statement_timeout. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sure. But isn't that testing Postgres itself? The tests already confirmed that the setting is set on the connection. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
You may have a point here, but the test seems pretty simple to write, so IMHO it's better to add it. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I've written it. Travis is running so I'm hoping the error response from postgres is the same from all the versions you guys support. I asserted the error code only with the hope that is will be less fragile than expecting a particular error message. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks like an unwanted edit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've added a newline to the end of the file to remove this change