From 6eeb949583b84da34ba5a8edff2f1251e0f5e9cc Mon Sep 17 00:00:00 2001 From: Rafa de la Torre Date: Thu, 19 Jul 2018 17:27:24 +0200 Subject: [PATCH 1/6] This is a bash script #521 And using sh can make some things fail. E.g: ``` [[: not found ``` --- test/run_tests.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/run_tests.sh b/test/run_tests.sh index c3f2cb67..354efa07 100755 --- a/test/run_tests.sh +++ b/test/run_tests.sh @@ -1,4 +1,4 @@ -#!/bin/sh +#!/bin/bash # To make output dates deterministic export TZ='Europe/Rome' From 20d1db78dc04775658bf301172cea1a5ed8d3d8f Mon Sep 17 00:00:00 2001 From: Rafa de la Torre Date: Thu, 19 Jul 2018 17:30:05 +0200 Subject: [PATCH 2/6] Helper to reset pgbouncer connections #521 This sends a PAUSE and RESUME to pgbouncer (in case there's one) before and after executing tests, to make sure new connections are established in the tests. This may be especially important when role or session settings are modified in the DB (same happens in prod, BTW). --- test/acceptance/batch/batch-limits.test.js | 4 ++- test/acceptance/export/timeout.js | 4 +++ test/support/db_utils.js | 37 ++++++++++++++++++++++ 3 files changed, 44 insertions(+), 1 deletion(-) create mode 100644 test/support/db_utils.js diff --git a/test/acceptance/batch/batch-limits.test.js b/test/acceptance/batch/batch-limits.test.js index f015787e..5d46872c 100644 --- a/test/acceptance/batch/batch-limits.test.js +++ b/test/acceptance/batch/batch-limits.test.js @@ -5,6 +5,7 @@ var BatchTestClient = require('../../support/batch-test-client'); var JobStatus = require('../../../batch/job_status'); var redisUtils = require('../../support/redis_utils'); var metadataBackend = require('cartodb-redis')({ pool: redisUtils.getPool() }); +const db_utils = require('../../support/db_utils'); describe('batch query statement_timeout limit', function() { @@ -14,13 +15,14 @@ describe('batch query statement_timeout limit', function() { global.settings.batch_query_timeout = 15000; metadataBackend.redisCmd(5, 'HMSET', ['limits:batch:vizzuality', 'timeout', 100], done); }); - + before(db_utils.resetPgBouncerConnections); after(function(done) { global.settings.batch_query_timeout = this.batchQueryTimeout; redisUtils.clean('limits:batch:*', function() { this.batchTestClient.drain(done); }.bind(this)); }); + after(db_utils.resetPgBouncerConnections); function jobPayload(query) { return { diff --git a/test/acceptance/export/timeout.js b/test/acceptance/export/timeout.js index 0a927acc..2cc5a558 100644 --- a/test/acceptance/export/timeout.js +++ b/test/acceptance/export/timeout.js @@ -4,9 +4,13 @@ require('../../support/assert'); var assert = require('assert'); var querystring = require('querystring'); +const db_utils = require('../../support/db_utils'); describe('timeout', function () { describe('export database', function () { + before(db_utils.resetPgBouncerConnections); + after(db_utils.resetPgBouncerConnections); + const databaseTimeoutQuery = ` select ST_SetSRID(ST_Point(0, 0), 4326) as the_geom, diff --git a/test/support/db_utils.js b/test/support/db_utils.js new file mode 100644 index 00000000..e747a7f1 --- /dev/null +++ b/test/support/db_utils.js @@ -0,0 +1,37 @@ +'use strict'; + +const { Client } = require('pg'); + +const dbConfig = { + db_user: process.env.PGUSER || 'postgres', + db_host: global.settings.db_host, + db_port: global.settings.db_port, + db_batch_port: global.settings.db_batch_port +}; + +module.exports.resetPgBouncerConnections = function (callback) { + // We assume there's no pgbouncer if db_port === db_batch_port + if (dbConfig.db_port === dbConfig.db_batch_port) { + return callback(); + } + + const client = new Client({ + database: 'pgbouncer', + user: dbConfig.db_user, + host: dbConfig.db_host, + port: dbConfig.db_port + }); + + // We just chain a PAUSE followed by a RESUME + client.connect(); + client.query('PAUSE', (err, res) => { + if (err) { + return callback(err); + } else { + client.query('RESUME', (err, res) => { + client.end(); + return callback(err); + }); + } + }); +} From 5b65252c4a3296374d499c4174c912f93873a5b2 Mon Sep 17 00:00:00 2001 From: Rafa de la Torre Date: Thu, 19 Jul 2018 18:38:17 +0200 Subject: [PATCH 3/6] Ensure no connection pgbouncer -> test_db #521 This is needed to make the setup a little bit more robust: when trying to delete the test database, it won't be able if there are "idle sessions" in the db (that is, connections from pgbouncer to the test database). Otherwise it fails when trying to create the database, because there's already one with the same name. --- test/prepare_db.sh | 1 + 1 file changed, 1 insertion(+) diff --git a/test/prepare_db.sh b/test/prepare_db.sh index c5e92015..df0220dc 100755 --- a/test/prepare_db.sh +++ b/test/prepare_db.sh @@ -70,6 +70,7 @@ if test x"$PREPARE_PGSQL" = xyes; then echo "preparing postgres..." echo "PostgreSQL server version: `psql -A -t -c 'select version()'`" + echo "PAUSE; RESUME;" | psql -p 6432 pgbouncer # make sure there are no connections pgbouncer -> test_db dropdb ${TEST_DB} # 2> /dev/null # error expected if doesn't exist, but not otherwise createdb -Ttemplate_postgis -EUTF8 ${TEST_DB} || die "Could not create test database" psql -c 'CREATE EXTENSION IF NOT EXISTS "uuid-ossp";' ${TEST_DB} From bf1d5bf3b4227b52d66ec1617b27a349caa822a3 Mon Sep 17 00:00:00 2001 From: Rafa de la Torre Date: Mon, 30 Jul 2018 12:31:57 +0200 Subject: [PATCH 4/6] Early return instead of if/else As suggested in review comment. --- test/support/db_utils.js | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/test/support/db_utils.js b/test/support/db_utils.js index e747a7f1..cc990a6a 100644 --- a/test/support/db_utils.js +++ b/test/support/db_utils.js @@ -27,11 +27,10 @@ module.exports.resetPgBouncerConnections = function (callback) { client.query('PAUSE', (err, res) => { if (err) { return callback(err); - } else { - client.query('RESUME', (err, res) => { - client.end(); - return callback(err); - }); } + client.query('RESUME', (err, res) => { + client.end(); + return callback(err); + }); }); } From 2afe845d7800ea6e5ffbd09362cef0415d65ad22 Mon Sep 17 00:00:00 2001 From: Rafa de la Torre Date: Mon, 30 Jul 2018 12:34:29 +0200 Subject: [PATCH 5/6] More descriptive comment about the intent of the code As suggested in review comment. --- test/support/db_utils.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/support/db_utils.js b/test/support/db_utils.js index cc990a6a..143eee61 100644 --- a/test/support/db_utils.js +++ b/test/support/db_utils.js @@ -22,7 +22,7 @@ module.exports.resetPgBouncerConnections = function (callback) { port: dbConfig.db_port }); - // We just chain a PAUSE followed by a RESUME + // We just chain a PAUSE followed by a RESUME to reset internal pool connections of PgBouncer client.connect(); client.query('PAUSE', (err, res) => { if (err) { From 942b0073d8c4ae428587c4a44c5d5f203feffeff Mon Sep 17 00:00:00 2001 From: Rafa de la Torre Date: Mon, 30 Jul 2018 12:36:16 +0200 Subject: [PATCH 6/6] Please jshint As suggested by jshint: - Remove the unused `res` return value - Add a semicolon at the end of the file --- test/support/db_utils.js | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/test/support/db_utils.js b/test/support/db_utils.js index 143eee61..decfc9fc 100644 --- a/test/support/db_utils.js +++ b/test/support/db_utils.js @@ -24,13 +24,13 @@ module.exports.resetPgBouncerConnections = function (callback) { // We just chain a PAUSE followed by a RESUME to reset internal pool connections of PgBouncer client.connect(); - client.query('PAUSE', (err, res) => { + client.query('PAUSE', err => { if (err) { return callback(err); } - client.query('RESUME', (err, res) => { + client.query('RESUME', err => { client.end(); return callback(err); }); }); -} +};