521 test robustness pgbouncer #522

rafatower · 2018-07-19T16:49:13Z

This PR makes it possible to execute tests with and without pgbouncer.

This fixes the issue #521

And using sh can make some things fail. E.g: ``` [[: not found ```

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).

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.

…-pgbouncer

rafatower · 2018-07-25T13:19:53Z

I understand that for this to be really more automated and useful, I'd need to tweak the travis build and docker image to make use of pgbouncer, another dependency, and that it might not be desirable.

No big deal and no strong opinion, it is simply that it helped me to run existing tests, taking into account subtle differences between direct vs pgbounced connections.

simon-contreras-deel · 2018-07-26T16:31:06Z

test/support/db_utils.js

+        port: dbConfig.db_port
+    });
+
+    // We just chain a PAUSE followed by a RESUME


Ei, what is the objective of pause - resume?

https://pgbouncer.github.io/usage.html#admin-console

PAUSE [db];
PgBouncer tries to disconnect from all servers, first waiting for all queries to complete. The command will not return before all queries are finished. To be used at the time of database restart.

RESUME [db];
Resume work from previous PAUSE or SUSPEND command.

Wait for all the running queries to finish and let the new ones reconnect. This way you pick the changed session parameters such as set statement_timeout. Sames happens in production, BTW, but there the connections are refreshed usually more often.

Ok, thanks! :)

dgaubert

LGTM, just leave a couple of picky comments for the sake of clarity :)

Is this also happening in your environment for Windshaft-cartodb??

dgaubert · 2018-07-30T07:33:57Z

test/support/db_utils.js

+
+    // We just chain a PAUSE followed by a RESUME
+    client.connect();
+    client.query('PAUSE', (err, res) => {


Please, follow Node.js conventions:

client.query('PAUSE', (err, res) => { if (err) { return callback(err); } client.query('RESUME', (err, res) => ...); }

If you inmediatly return after calling the callback, there is no need to wrap following sentences into a else statement.

dgaubert · 2018-07-30T07:40:56Z

test/support/db_utils.js

+        port: dbConfig.db_port
+    });
+
+    // We just chain a PAUSE followed by a RESUME


What about // We just chain a PAUSE followed by a RESUME to reset internal pool connections of PgBouncer?

rafatower · 2018-07-30T10:28:11Z

Is this also happening in your environment for Windshaft-cartodb?

Windshaft-cartodb is equally affected, but its tests don't need much of DB settings as those in SQL API:

[rtorre@fireflies Windshaft-cartodb (master)]$ git grep statement_timeout
test/support/sql/gadm4.sql:SET statement_timeout = 0;
test/support/sql/windshaft.test.sql:SET statement_timeout = 0;

Yet you can see some tests are indeed affected by this:

[rtorre@fireflies Windshaft-cartodb (master)]$ diff config/environments/test.js config/environments/test.js.example
106c106
<         port: 6432,
---
>         port: 5432,
116c116
<     ,mapnik_version: ''
---
>     ,mapnik_version: undefined
193c193
<               port: 6432,
---
>               port: 5432,

[rtorre@fireflies Windshaft-cartodb (master)]$ make check
...
  443 passing (31s)
  8 pending
  2 failing

  1) user database timeout limit dataview layergroup creation works but dataview request fails due to statement timeout:
     Uncaught Error: Invalid response status code.
     Expected: 429
     Got: 200
     Body: {"operation":"count","result":1,"nulls":0,"type":"formula"}
      at validateResponseStatus (test/support/assert.js:146:20)
      at validateResponse (test/support/assert.js:179:9)
      at Server.<anonymous> (test/support/assert.js:119:27)
      at emitCloseNT (net.js:1555:8)
      at _combinedTickCallback (internal/process/next_tick.js:71:11)
      at process._tickCallback (internal/process/next_tick.js:98:9)

  2)  "after each" hook for "layergroup creation works but dataview request fails due to statement timeout":

      AssertionError: uncaughtException:

Error: Invalid response status code.
     Expected: 429
     Got: 200
     Body: {"operation":"count","result":1,"nulls":0,"type":"formula"}
    at validateResponseStatus (test/support/assert.js:146:20)
    at validateResponse (test/support/assert.js:179:9)
    at Server.<anonymous> (test/support/assert.js:119:27)
    at emitCloseNT (net.js:1555:8)
    at _combinedTickCallback (internal/process/next_tick.js:71:11)
    at process._tickCallback (internal/process/next_tick.js:98:9)
      + expected - actual

      -1
      +0
      
      at Context.<anonymous> (test/support/test_helper.js:87:12)

(if I use the standard 5432 port, all of them pass).

Arguably, I'd be less concerned about Windshaft-cartodb than about the SQL API. In the end, the SQL API is all about interaction with the DB, and the direct connection vs pgbouncer is more than just an implementation detail.

As suggested in review comment.

As suggested by jshint: - Remove the unused `res` return value - Add a semicolon at the end of the file

rafatower · 2018-07-30T10:47:38Z

I covered your comments (that I always appreciate) and pleased jshint. Now time to merge. Thanks!

simon-contreras-deel · 2018-07-30T10:50:47Z

👍

Rafa de la Torre added 3 commits July 19, 2018 18:40

This is a bash script #521

6eeb949

And using sh can make some things fail. E.g: ``` [[: not found ```

rafatower requested a review from dgaubert July 19, 2018 16:59

Merge remote-tracking branch 'origin/master' into 521-test-robustness…

7277143

…-pgbouncer

rafatower requested review from simon-contreras-deel and enekid July 25, 2018 13:16

simon-contreras-deel reviewed Jul 26, 2018

View reviewed changes

dgaubert approved these changes Jul 30, 2018

View reviewed changes

Rafa de la Torre added 3 commits July 30, 2018 12:31

Early return instead of if/else

bf1d5bf

As suggested in review comment.

More descriptive comment about the intent of the code

2afe845

As suggested in review comment.

Please jshint

942b007

As suggested by jshint: - Remove the unused `res` return value - Add a semicolon at the end of the file

rafatower merged commit bdb3b9f into master Jul 30, 2018

rafatower deleted the 521-test-robustness-pgbouncer branch July 30, 2018 10:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

521 test robustness pgbouncer #522

521 test robustness pgbouncer #522

rafatower commented Jul 19, 2018 •

edited

Loading

rafatower commented Jul 25, 2018

simon-contreras-deel Jul 26, 2018

rafatower Jul 26, 2018

simon-contreras-deel Jul 26, 2018

dgaubert left a comment

dgaubert Jul 30, 2018

dgaubert Jul 30, 2018

rafatower commented Jul 30, 2018

rafatower commented Jul 30, 2018

simon-contreras-deel commented Jul 30, 2018

521 test robustness pgbouncer #522

521 test robustness pgbouncer #522

Conversation

rafatower commented Jul 19, 2018 • edited Loading

rafatower commented Jul 25, 2018

simon-contreras-deel Jul 26, 2018

Choose a reason for hiding this comment

rafatower Jul 26, 2018

Choose a reason for hiding this comment

simon-contreras-deel Jul 26, 2018

Choose a reason for hiding this comment

dgaubert left a comment

Choose a reason for hiding this comment

dgaubert Jul 30, 2018

Choose a reason for hiding this comment

dgaubert Jul 30, 2018

Choose a reason for hiding this comment

rafatower commented Jul 30, 2018

rafatower commented Jul 30, 2018

simon-contreras-deel commented Jul 30, 2018

rafatower commented Jul 19, 2018 •

edited

Loading