From 837b591cceee8650fb40165dd396d9536f8e814a Mon Sep 17 00:00:00 2001 From: Pablo Palacios Date: Mon, 18 Nov 2024 19:07:25 +0100 Subject: [PATCH 1/3] tests: cleanup testCrud code --- tests/util/testCrud.js | 158 +++++++++++++++++++++++++---------------- 1 file changed, 96 insertions(+), 62 deletions(-) diff --git a/tests/util/testCrud.js b/tests/util/testCrud.js index 66fefaa..2c361c5 100644 --- a/tests/util/testCrud.js +++ b/tests/util/testCrud.js @@ -1,9 +1,9 @@ -var expect = require('chai').expect; -var defaultOptions = require('./defaultOptions'); -var resource = defaultOptions.resource; -var invalidResource = 'invalid_resource'; -var mockErrorService = require('../mock/MockErrorService'); -var mockNoopService = require('../mock/MockNoopService'); +const expect = require('chai').expect; +const defaultOptions = require('./defaultOptions'); +const resource = defaultOptions.resource; +const invalidResource = 'invalid_resource'; +const mockErrorService = require('../mock/MockErrorService'); +const mockNoopService = require('../mock/MockNoopService'); module.exports = function testCrud( params, @@ -13,7 +13,8 @@ module.exports = function testCrud( resolve, reject, ) { - var options = {}; + let options = {}; + if (arguments.length === 1) { options = params; params = options.params || defaultOptions.params; @@ -23,39 +24,44 @@ module.exports = function testCrud( resolve = options.resolve || defaultOptions.resolve; reject = options.reject || defaultOptions.reject; } + describe('CRUD Interface', function () { describe('should work superagent style', function () { describe('with callbacks', function () { it('should handle CREATE', function (done) { - var operation = 'create'; + const operation = 'create'; this.fetcher[operation](resource) .params(params) .body(body) .clientConfig(config) .end(callback(operation, done)); }); + it('should handle READ', function (done) { - var operation = 'read'; + const operation = 'read'; this.fetcher[operation](resource) .params(params) .clientConfig(config) .end(callback(operation, done)); }); + it('should handle UPDATE', function (done) { - var operation = 'update'; + const operation = 'update'; this.fetcher[operation](resource) .params(params) .body(body) .clientConfig(config) .end(callback(operation, done)); }); + it('should handle DELETE', function (done) { - var operation = 'delete'; + const operation = 'delete'; this.fetcher[operation](resource) .params(params) .clientConfig(config) .end(callback(operation, done)); }); + it('should throw if no resource is given', function () { expect(this.fetcher.read.bind(this.fetcher)).to.throw( 'Resource is required for a fetcher request', @@ -64,8 +70,22 @@ module.exports = function testCrud( }); describe('with Promises', function () { + function denySuccess(done) { + return function () { + done(new Error('This operation should have failed')); + }; + } + + function allowFailure(done) { + return function (err) { + expect(err.name).to.equal('FetchrError'); + expect(err.message).to.exist; + done(); + }; + } + it('should handle CREATE', function (done) { - var operation = 'create'; + const operation = 'create'; this.fetcher[operation](resource) .params(params) .body(body) @@ -75,8 +95,9 @@ module.exports = function testCrud( reject(operation, done), ); }); + it('should handle READ', function (done) { - var operation = 'read'; + const operation = 'read'; this.fetcher[operation](resource) .params(params) .clientConfig(config) @@ -85,8 +106,9 @@ module.exports = function testCrud( reject(operation, done), ); }); + it('should handle UPDATE', function (done) { - var operation = 'update'; + const operation = 'update'; this.fetcher[operation](resource) .params(params) .body(body) @@ -96,8 +118,9 @@ module.exports = function testCrud( reject(operation, done), ); }); + it('should handle DELETE', function (done) { - var operation = 'delete'; + const operation = 'delete'; this.fetcher[operation](resource) .params(params) .clientConfig(config) @@ -106,48 +129,41 @@ module.exports = function testCrud( reject(operation, done), ); }); - var denySuccess = function (done) { - return function () { - done(new Error('This operation should have failed')); - }; - }; - var allowFailure = function (done) { - return function (err) { - expect(err.name).to.equal('FetchrError'); - expect(err.message).to.exist; - done(); - }; - }; + it('should reject a CREATE promise on invalid resource', function (done) { - var operation = 'create'; + const operation = 'create'; this.fetcher[operation](invalidResource) .params(params) .body(body) .clientConfig(config) .then(denySuccess(done), allowFailure(done)); }); + it('should reject a READ promise on invalid resource', function (done) { - var operation = 'read'; + const operation = 'read'; this.fetcher[operation](invalidResource) .params(params) .clientConfig(config) .then(denySuccess(done), allowFailure(done)); }); + it('should reject a UPDATE promise on invalid resource', function (done) { - var operation = 'update'; + const operation = 'update'; this.fetcher[operation](invalidResource) .params(params) .body(body) .clientConfig(config) .then(denySuccess(done), allowFailure(done)); }); + it('should reject a DELETE promise on invalid resource', function (done) { - var operation = 'delete'; + const operation = 'delete'; this.fetcher[operation](invalidResource) .params(params) .clientConfig(config) .then(denySuccess(done), allowFailure(done)); }); + it('should throw if no resource is given', function () { expect(this.fetcher.read.bind(this.fetcher)).to.throw( 'Resource is required for a fetcher request', @@ -155,10 +171,23 @@ module.exports = function testCrud( }); }); }); + describe('should be backwards compatible', function () { + function denySuccess(done) { + return function (err) { + if (!err) { + done(new Error('This operation should have failed')); + } else { + expect(err.name).to.equal('FetchrError'); + expect(err.message).to.exist; + done(); + } + }; + } + // with config it('should handle CREATE', function (done) { - var operation = 'create'; + const operation = 'create'; this.fetcher[operation]( resource, params, @@ -167,8 +196,9 @@ module.exports = function testCrud( callback(operation, done), ); }); + it('should handle READ', function (done) { - var operation = 'read'; + const operation = 'read'; this.fetcher[operation]( resource, params, @@ -176,8 +206,9 @@ module.exports = function testCrud( callback(operation, done), ); }); + it('should handle UPDATE', function (done) { - var operation = 'update'; + const operation = 'update'; this.fetcher[operation]( resource, params, @@ -186,8 +217,9 @@ module.exports = function testCrud( callback(operation, done), ); }); + it('should handle DELETE', function (done) { - var operation = 'delete'; + const operation = 'delete'; this.fetcher[operation]( resource, params, @@ -195,19 +227,9 @@ module.exports = function testCrud( callback(operation, done), ); }); - var denySuccess = function (done) { - return function (err) { - if (!err) { - done(new Error('This operation should have failed')); - } else { - expect(err.name).to.equal('FetchrError'); - expect(err.message).to.exist; - done(); - } - }; - }; + it('should throw catchable error on CREATE with invalid resource', function (done) { - var operation = 'create'; + const operation = 'create'; this.fetcher[operation]( invalidResource, params, @@ -216,8 +238,9 @@ module.exports = function testCrud( denySuccess(done), ); }); + it('should throw catchable error on READ with invalid resource', function (done) { - var operation = 'read'; + const operation = 'read'; this.fetcher[operation]( invalidResource, params, @@ -225,8 +248,9 @@ module.exports = function testCrud( denySuccess(done), ); }); + it('should throw catchable error on UPDATE with invalid resource', function (done) { - var operation = 'update'; + const operation = 'update'; this.fetcher[operation]( invalidResource, params, @@ -235,8 +259,9 @@ module.exports = function testCrud( denySuccess(done), ); }); + it('should throw catchable error on DELETE with invalid resource', function (done) { - var operation = 'delete'; + const operation = 'delete'; this.fetcher[operation]( invalidResource, params, @@ -244,12 +269,13 @@ module.exports = function testCrud( denySuccess(done), ); }); + if (!options.disableNoConfigTests) { // without config // we have a feature flag to disable these tests because // it doesn't make sense to test a feature like CORS without being able to pass in a config it('should handle CREATE w/ no config', function (done) { - var operation = 'create'; + const operation = 'create'; this.fetcher[operation]( resource, params, @@ -257,16 +283,18 @@ module.exports = function testCrud( callback(operation, done), ); }); + it('should handle READ w/ no config', function (done) { - var operation = 'read'; + const operation = 'read'; this.fetcher[operation]( resource, params, callback(operation, done), ); }); + it('should handle UPDATE w/ no config', function (done) { - var operation = 'update'; + const operation = 'update'; this.fetcher[operation]( resource, params, @@ -274,8 +302,9 @@ module.exports = function testCrud( callback(operation, done), ); }); + it('should handle DELETE w/ no config', function (done) { - var operation = 'delete'; + const operation = 'delete'; this.fetcher[operation]( resource, params, @@ -284,8 +313,9 @@ module.exports = function testCrud( }); } }); + it('should keep track of metadata in getServiceMeta', function (done) { - var fetcher = this.fetcher; + const fetcher = this.fetcher; fetcher._serviceMeta.length = 0; // reset serviceMeta to empty array fetcher .read(resource) @@ -315,7 +345,7 @@ module.exports = function testCrud( expect(meta).to.include.keys('headers'); expect(meta.headers).to.include.keys('x-bar'); expect(meta.headers['x-bar']).to.equal('bar'); - var serviceMeta = fetcher.getServiceMeta(); + const serviceMeta = fetcher.getServiceMeta(); expect(serviceMeta).to.have.length(2); expect(serviceMeta[0].headers).to.include.keys( 'x-foo', @@ -333,9 +363,10 @@ module.exports = function testCrud( }); }); }); + describe('should have serviceMeta data on error', function () { it('with callbacks', function (done) { - var fetcher = this.fetcher; + const fetcher = this.fetcher; fetcher._serviceMeta.length = 0; // reset serviceMeta to empty array fetcher .read(mockErrorService.resource) @@ -346,7 +377,7 @@ module.exports = function testCrud( .clientConfig(config) .end(function (err) { if (err) { - var serviceMeta = fetcher.getServiceMeta(); + const serviceMeta = fetcher.getServiceMeta(); expect(serviceMeta).to.have.length(1); expect(serviceMeta[0]).to.include.keys('headers'); expect(serviceMeta[0].headers).to.include.keys( @@ -359,8 +390,9 @@ module.exports = function testCrud( } }); }); + it('with Promises', function (done) { - var fetcher = this.fetcher; + const fetcher = this.fetcher; fetcher._serviceMeta.length = 0; // reset serviceMeta to empty array fetcher .read(mockErrorService.resource) @@ -371,7 +403,7 @@ module.exports = function testCrud( .clientConfig(config) .catch(function (err) { if (err) { - var serviceMeta = fetcher.getServiceMeta(); + const serviceMeta = fetcher.getServiceMeta(); expect(serviceMeta).to.have.length(1); expect(serviceMeta[0]).to.include.keys('headers'); expect(serviceMeta[0].headers).to.include.keys( @@ -386,9 +418,10 @@ module.exports = function testCrud( }); }); }); + describe('should reject no operation service', function () { it('with callback', function (done) { - var fetcher = this.fetcher; + const fetcher = this.fetcher; fetcher .read(mockNoopService.resource) .clientConfig(config) @@ -400,8 +433,9 @@ module.exports = function testCrud( done(); }); }); + it('with Promise', function (done) { - var fetcher = this.fetcher; + const fetcher = this.fetcher; fetcher .read(mockNoopService.resource) .clientConfig(config) From 4680f93056ca94b63e83c43d13706e6584871efe Mon Sep 17 00:00:00 2001 From: Pablo Palacios Date: Mon, 18 Nov 2024 19:34:59 +0100 Subject: [PATCH 2/3] tests: use only one config argument for testCrud --- tests/unit/libs/fetcher.client.js | 153 +++++++++++++++++++++--------- tests/unit/libs/fetcher.js | 11 ++- tests/util/testCrud.js | 31 ++---- 3 files changed, 128 insertions(+), 67 deletions(-) diff --git a/tests/unit/libs/fetcher.client.js b/tests/unit/libs/fetcher.client.js index faaa953..1c20fd1 100644 --- a/tests/unit/libs/fetcher.client.js +++ b/tests/unit/libs/fetcher.client.js @@ -121,10 +121,20 @@ describe('Client Fetcher', function () { } }; }); + beforeEach(function () { stats = null; }); - testCrud(params, body, config, callbackWithStats, resolve, reject); + + testCrud({ + params, + body, + config, + callback: callbackWithStats, + reject, + resolve, + }); + after(function () { validateRequest = null; }); @@ -147,15 +157,17 @@ describe('Client Fetcher', function () { corsPath: corsPath, }); }); + testCrud({ - params: params, - body: body, + params, + body, config: { cors: true }, + callback, + reject, + resolve, disableNoConfigTests: true, - callback: callback, - resolve: resolve, - reject: reject, }); + after(function () { validateRequest = null; }); @@ -222,7 +234,16 @@ describe('Client Fetcher', function () { xhrTimeout: 4000, }); }); - testCrud(params, body, config, callback, resolve, reject); + + testCrud({ + params, + body, + config, + callback, + resolve, + reject, + }); + after(function () { mockery.deregisterMock('./util/httpRequest'); mockery.disable(); @@ -245,17 +266,17 @@ describe('Client Fetcher', function () { xhrTimeout: 4000, }); }); + testCrud({ - params: params, - body: body, - config: { - timeout: 5000, - }, - disableNoConfigTests: true, + params, + body, + config: { timeout: 5000 }, callback: callback, resolve: resolve, reject: reject, + disableNoConfigTests: true, }); + after(function () { mockery.deregisterMock('./util/httpRequest'); mockery.disable(); @@ -277,7 +298,16 @@ describe('Client Fetcher', function () { context: context, }); }); - testCrud(params, body, config, callback, resolve, reject); + + testCrud({ + params, + body, + config, + callback, + resolve, + reject, + }); + after(function () { mockery.deregisterMock('./util/httpRequest'); mockery.disable(); @@ -325,7 +355,14 @@ describe('Client Fetcher', function () { }); }); - testCrud(params, body, config, callback, resolve, reject); + testCrud({ + params, + body, + config, + callback, + resolve, + reject, + }); }); describe('Property Name', function () { @@ -338,7 +375,14 @@ describe('Client Fetcher', function () { }); }); - testCrud(params, body, config, callback, resolve, reject); + testCrud({ + params, + body, + config, + callback, + resolve, + reject, + }); }); describe('Property Names', function () { @@ -351,7 +395,14 @@ describe('Client Fetcher', function () { }); }); - testCrud(params, body, config, callback, resolve, reject); + testCrud({ + params, + body, + config, + callback, + resolve, + reject, + }); }); }); @@ -396,7 +447,16 @@ describe('Client Fetcher', function () { }, }); }); - testCrud(params, body, config, callback, resolve, reject); + + testCrud({ + params, + body, + config, + callback, + resolve, + reject, + }); + after(function () { mockery.deregisterMock('./util/httpRequest'); mockery.disable(); @@ -418,20 +478,17 @@ describe('Client Fetcher', function () { context: context, }); }); - var customConfig = { - headers: { - 'X-APP-VERSION': VERSION, - }, - }; + testCrud({ + params, + body, + config: { headers: { 'X-APP-VERSION': VERSION } }, + callback, + resolve, + reject, disableNoConfigTests: true, - params: params, - body: body, - config: customConfig, - callback: callback, - resolve: resolve, - reject: reject, }); + after(function () { mockery.deregisterMock('./util/httpRequest'); mockery.disable(); @@ -679,7 +736,14 @@ describe('Client Fetcher', function () { }); }); - testCrud(params, body, config, callback, resolve, reject); + testCrud({ + params, + body, + config, + callback, + resolve, + reject, + }); after(function () { mockery.deregisterMock('./util/httpRequest'); @@ -705,23 +769,24 @@ describe('Client Fetcher', function () { Fetcher = require('../../../libs/fetcher.client'); this.fetcher = new Fetcher({}); }); - var customConfig = { - retry: { - interval: 350, - maxRetries: 2, - statusCodes: [0, 502, 504], - }, - unsafeAllowRetry: true, - }; + testCrud({ + params, + body, + config: { + retry: { + interval: 350, + maxRetries: 2, + statusCodes: [0, 502, 504], + }, + unsafeAllowRetry: true, + }, + callback, + resolve, + reject, disableNoConfigTests: true, - params: params, - body: body, - config: customConfig, - callback: callback, - resolve: resolve, - reject: reject, }); + after(function () { mockery.deregisterMock('./util/httpRequest'); mockery.disable(); diff --git a/tests/unit/libs/fetcher.js b/tests/unit/libs/fetcher.js index cd36330..0edcb53 100644 --- a/tests/unit/libs/fetcher.js +++ b/tests/unit/libs/fetcher.js @@ -1293,8 +1293,15 @@ describe('Server Fetcher', function () { beforeEach(function () { stats = null; }); - // CRUD - testCrud(params, body, config, callbackWithStats, resolve, reject); + + testCrud({ + params, + body, + config, + callback: callbackWithStats, + resolve, + reject, + }); }); }); diff --git a/tests/util/testCrud.js b/tests/util/testCrud.js index 2c361c5..57478cb 100644 --- a/tests/util/testCrud.js +++ b/tests/util/testCrud.js @@ -5,26 +5,15 @@ const invalidResource = 'invalid_resource'; const mockErrorService = require('../mock/MockErrorService'); const mockNoopService = require('../mock/MockNoopService'); -module.exports = function testCrud( - params, - body, - config, - callback, - resolve, - reject, -) { - let options = {}; - - if (arguments.length === 1) { - options = params; - params = options.params || defaultOptions.params; - body = options.body || defaultOptions.body; - config = options.config || defaultOptions.config; - callback = options.callback || defaultOptions.callback; - resolve = options.resolve || defaultOptions.resolve; - reject = options.reject || defaultOptions.reject; - } - +module.exports = function testCrud({ + params = defaultOptions.params, + body = defaultOptions.body, + config = defaultOptions.config, + callback = defaultOptions.callback, + resolve = defaultOptions.resolve, + reject = defaultOptions.reject, + disableNoConfigTests = false, +}) { describe('CRUD Interface', function () { describe('should work superagent style', function () { describe('with callbacks', function () { @@ -270,7 +259,7 @@ module.exports = function testCrud( ); }); - if (!options.disableNoConfigTests) { + if (!disableNoConfigTests) { // without config // we have a feature flag to disable these tests because // it doesn't make sense to test a feature like CORS without being able to pass in a config From fbbea28946b5e1b81ae324a6d58b3a859b3fa2d2 Mon Sep 17 00:00:00 2001 From: Pablo Palacios Date: Mon, 18 Nov 2024 19:37:06 +0100 Subject: [PATCH 3/3] fix: use 4xx errors for client errors --- libs/fetcher.js | 4 +++- tests/functional/app.js | 4 +++- tests/functional/fetchr.test.js | 28 ++++++++++++++++++++++++++++ tests/unit/libs/fetcher.js | 1 + tests/util/testCrud.js | 19 ++++++++++++++----- 5 files changed, 49 insertions(+), 7 deletions(-) diff --git a/libs/fetcher.js b/libs/fetcher.js index cb8b37d..9072e9f 100644 --- a/libs/fetcher.js +++ b/libs/fetcher.js @@ -252,8 +252,10 @@ class Request { if (!handler) { const err = new FetchrError( - `operation: ${this.operation} is undefined on service: ${this.resource}`, + `Operation "${this.operation}" is not allowed for resource "${sanitizeResourceName(this.resource)}"`, ); + err.statusCode = 405; + return { err }; } diff --git a/tests/functional/app.js b/tests/functional/app.js index 755ada3..b8a0dab 100644 --- a/tests/functional/app.js +++ b/tests/functional/app.js @@ -19,7 +19,9 @@ app.use(express.json()); app.use('/static', express.static(path.join(__dirname, 'static'))); -app.use('/api', Fetchr.middleware()); +app.use('/api', Fetchr.middleware(), (err, req, res, next) => { + res.status(err.statusCode || 500).json({ message: err.message }); +}); app.get('/', (req, res) => { res.send(` diff --git a/tests/functional/fetchr.test.js b/tests/functional/fetchr.test.js index 8e99625..0e611db 100644 --- a/tests/functional/fetchr.test.js +++ b/tests/functional/fetchr.test.js @@ -595,6 +595,34 @@ describe('client/server integration', () => { }); }); }); + + describe('client errors', () => { + it('handles unknown resources', async () => { + const response = await page.evaluate(() => { + const fetcher = new Fetchr(); + return fetcher + .create('unknown-resource') + .catch((err) => err); + }); + + expect(response.statusCode).to.equal(400); + expect(response.message).to.equal( + 'Resource "unknown*resource" is not registered', + ); + }); + + it('handles not implemented operations', async () => { + const response = await page.evaluate(() => { + const fetcher = new Fetchr(); + return fetcher.delete('error').catch((err) => err); + }); + + expect(response.statusCode).to.equal(405); + expect(JSON.parse(response.message).output.message).to.equal( + 'Operation "delete" is not allowed for resource "error"', + ); + }); + }); }); describe('headers', () => { diff --git a/tests/unit/libs/fetcher.js b/tests/unit/libs/fetcher.js index 0edcb53..ac705a3 100644 --- a/tests/unit/libs/fetcher.js +++ b/tests/unit/libs/fetcher.js @@ -1301,6 +1301,7 @@ describe('Server Fetcher', function () { callback: callbackWithStats, resolve, reject, + isServer: true, }); }); }); diff --git a/tests/util/testCrud.js b/tests/util/testCrud.js index 57478cb..2af2f64 100644 --- a/tests/util/testCrud.js +++ b/tests/util/testCrud.js @@ -13,6 +13,7 @@ module.exports = function testCrud({ resolve = defaultOptions.resolve, reject = defaultOptions.reject, disableNoConfigTests = false, + isServer = false, }) { describe('CRUD Interface', function () { describe('should work superagent style', function () { @@ -408,16 +409,23 @@ module.exports = function testCrud({ }); }); - describe('should reject no operation service', function () { + describe('should reject when resource does not implement operation', function () { + function getErrorMessage(err) { + return isServer + ? err.message + : JSON.parse(err.message).output.message; + } + it('with callback', function (done) { const fetcher = this.fetcher; fetcher .read(mockNoopService.resource) .clientConfig(config) .end(function (err) { + const message = getErrorMessage(err); expect(err.name).to.equal('FetchrError'); - expect(err.message).to.contain( - 'operation: read is undefined on service: mock_noop_service', + expect(message).to.equal( + 'Operation "read" is not allowed for resource "mock_noop_service"', ); done(); }); @@ -429,9 +437,10 @@ module.exports = function testCrud({ .read(mockNoopService.resource) .clientConfig(config) .catch(function (err) { + const message = getErrorMessage(err); expect(err.name).to.equal('FetchrError'); - expect(err.message).to.contain( - 'operation: read is undefined on service: mock_noop_service', + expect(message).to.equal( + 'Operation "read" is not allowed for resource "mock_noop_service"', ); done(); });