From f4e99629f613c52df8c079086ee7fe0c7c235f4e Mon Sep 17 00:00:00 2001 From: Raul Ochoa Date: Fri, 26 May 2017 13:02:02 +0200 Subject: [PATCH 1/3] Do not assert inside response, but pass error into callback Preferably we should put response outside of assert and change its callback signature. However, I don't think it is worth the effort right now. --- test/support/assert.js | 31 ++++++++++++++++++------------- 1 file changed, 18 insertions(+), 13 deletions(-) diff --git a/test/support/assert.js b/test/support/assert.js index a53d6fbf..c5e22e93 100644 --- a/test/support/assert.js +++ b/test/support/assert.js @@ -126,22 +126,25 @@ assert.response = function(server, req, res, callback) { // Assert response body if (res.body) { var eql = res.body instanceof RegExp ? res.body.test(response.body) : res.body === response.body; - assert.ok( - eql, - colorize('[red]{Invalid response body.}\n' + + if (!eql) { + return callback(response, new Error(colorize( + '[red]{Invalid response body.}\n' + ' Expected: [green]{' + res.body + '}\n' + - ' Got: [red]{' + response.body + '}') - ); + ' Got: [red]{' + response.body + '}')) + ); + } } // Assert response status if (typeof status === 'number') { - assert.equal(response.statusCode, status, - colorize('[red]{Invalid response status code.}\n' + + if (response.statusCode != status) { + return callback(response, new Error(colorize( + '[red]{Invalid response status code.}\n' + ' Expected: [green]{' + status + '}\n' + ' Got: [red]{' + response.statusCode + '}\n' + - ' Body: ' + response.body) - ); + ' Body: ' + response.body)) + ); + } } // Assert response headers @@ -152,11 +155,13 @@ assert.response = function(server, req, res, callback) { actual = response.headers[name.toLowerCase()], expected = res.headers[name], headerEql = expected instanceof RegExp ? expected.test(actual) : expected === actual; - assert.ok(headerEql, - colorize('Invalid response header [bold]{' + name + '}.\n' + + if (!headerEql) { + return callback(response, new Error(colorize( + 'Invalid response header [bold]{' + name + '}.\n' + ' Expected: [green]{' + expected + '}\n' + - ' Got: [red]{' + actual + '}') - ); + ' Got: [red]{' + actual + '}')) + ); + } } } From 248adab05ba4261478ba01b96ee61a31c87f3f16 Mon Sep 17 00:00:00 2001 From: Raul Ochoa Date: Fri, 26 May 2017 13:06:04 +0200 Subject: [PATCH 2/3] Catch response body if any to capture Redis keys --- test/support/test-client.js | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-) diff --git a/test/support/test-client.js b/test/support/test-client.js index bbb32c9d..04980a5a 100644 --- a/test/support/test-client.js +++ b/test/support/test-client.js @@ -617,18 +617,20 @@ TestClient.prototype.getLayergroup = function(expectedResponse, callback) { }, expectedResponse, function(res, err) { + // If there is a response, we are still interested in catching the created keys + // to be able to delete them on the .drain() method. + if (res) { + var parsedBody = JSON.parse(res.body); + if (parsedBody.layergroupid) { + self.keysToDelete['map_cfg|' + LayergroupToken.parse(parsedBody.layergroupid).token] = 0; + self.keysToDelete['user:localhost:mapviews:global'] = 5; + } + } if (err) { return callback(err); } - var parsedBody = JSON.parse(res.body); - - if (parsedBody.layergroupid) { - self.keysToDelete['map_cfg|' + LayergroupToken.parse(parsedBody.layergroupid).token] = 0; - self.keysToDelete['user:localhost:mapviews:global'] = 5; - } - - return callback(null, parsedBody); + return callback(null, JSON.parse(res.body)); } ); }; From 882aeacac23204f6b79c38e3fe6738073f05f8d7 Mon Sep 17 00:00:00 2001 From: Raul Ochoa Date: Fri, 26 May 2017 13:13:19 +0200 Subject: [PATCH 3/3] Rewrite test to take advantage of changes in assert.response/TestClient This should avoid the issue of preventing the whole suite to halt, as in https://travis-ci.org/CartoDB/Windshaft-cartodb/builds/236337027. --- .../ported/multilayer_error_cases.js | 37 ++++++++++--------- 1 file changed, 19 insertions(+), 18 deletions(-) diff --git a/test/acceptance/ported/multilayer_error_cases.js b/test/acceptance/ported/multilayer_error_cases.js index f0464699..3408baa2 100644 --- a/test/acceptance/ported/multilayer_error_cases.js +++ b/test/acceptance/ported/multilayer_error_cases.js @@ -5,6 +5,7 @@ var step = require('step'); var cartodbServer = require('../../../lib/cartodb/server'); var ServerOptions = require('./support/ported_server_options'); var testClient = require('./support/test_client'); +var TestClient = require('../../support/test-client'); var BaseController = require('../../../lib/cartodb/controllers/base'); @@ -23,6 +24,14 @@ describe('multilayer error cases', function() { BaseController.prototype.req2params = req2paramsFn; }); + // var client = null; + afterEach(function(done) { + if (this.client) { + return this.client.drain(done); + } + return done(); + }); + it("post layergroup with wrong Content-Type", function(done) { assert.response(server, { url: '/database/windshaft_test/layergroup', @@ -153,24 +162,16 @@ describe('multilayer error cases', function() { ] }; ServerOptions.afterLayergroupCreateCalls = 0; - assert.response(server, { - url: '/database/windshaft_test/layergroup', - method: 'POST', - headers: {'Content-Type': 'application/json' }, - data: JSON.stringify(layergroup) - }, {}, function(res) { - try { - assert.equal(res.statusCode, 400, res.statusCode + ': ' + res.body); - // See http://github.com/CartoDB/Windshaft/issues/159 - assert.equal(ServerOptions.afterLayergroupCreateCalls, 0); - var parsed = JSON.parse(res.body); - assert.ok(parsed); - assert.equal(parsed.errors.length, 1); - var error = parsed.errors[0]; - assert.ok(error.match(/column "missing" does not exist/m), error); - // TODO: check which layer introduced the problem ? - done(); - } catch (err) { done(err); } + this.client = new TestClient(layergroup); + this.client.getLayergroup({status: 400}, function(err, parsed) { + assert.ok(!err, err); + // See http://github.com/CartoDB/Windshaft/issues/159 + assert.equal(ServerOptions.afterLayergroupCreateCalls, 0); + assert.ok(parsed); + assert.equal(parsed.errors.length, 1); + var error = parsed.errors[0]; + assert.ok(error.match(/column "missing" does not exist/m), error); + done(); }); });