rabbah commented on a change in pull request #227: URL: https://github.com/apache/openwhisk-client-js/pull/227#discussion_r728930963
########## File path: README.md ########## @@ -105,7 +105,7 @@ _Client constructor supports the following mandatory parameters:_ - **key**. Client key to use when connecting to the `apihost` (if `nginx_ssl_verify_client` is turned on in your apihost) - **proxy.** HTTP(s) URI for proxy service to forwards requests through. Uses Needle's [built-in proxy support](https://github.com/tomas/needle#request-options). - **agent.** Provide custom [http.Agent](https://nodejs.org/api/http.html#http_class_http_agent) implementation. - +- **retry**. Provide a retry options to retry on errors, for example, `{ retries: 2 }`. By default, no retries will be done. Uses [async-retry options](https://github.com/vercel/async-retry#api). Default values are different from async-retry, please refer to the api doc. Review comment: ```suggestion - **retry**. Provide a retry options to retry on errors, for example, `{ retries: 2 }`. By default, no retries will be done. Uses [async-retry options](https://github.com/vercel/async-retry#api). Default values are different from async-retry, please refer to the API doc. ``` ########## File path: test/unit/client.test.js ########## @@ -22,18 +22,60 @@ const Client = require('../../lib/client') const http = require('http') const nock = require('nock') -// Note: this has to come before any of the proxy tests, as they interfere +// Note: All client.request tests have to come before any of the proxy tests, as they interfere + +test('should return response', async t => { + const client = new Client({ api_key: 'secret', apihost: 'test_host', proxy: '' }) + const METHOD = 'GET' + const PATH = '/some/path' + + const interceptor = nock('https://test_host').get(PATH).times(1).reply(200, 'all good') + const result = await client.request(METHOD, PATH, {}) + t.is(result.toString(), 'all good') + nock.removeInterceptor(interceptor) +}) + test('should handle http request errors', async t => { const client = new Client({ api_key: 'secret', apihost: 'test_host', proxy: '' }) const METHOD = 'GET' const PATH = '/some/path' - nock('https://test_host').get(PATH).replyWithError('simulated error') + const interceptor = nock('https://test_host').get(PATH).times(1).replyWithError('simulated error') const error = await t.throwsAsync(client.request(METHOD, PATH, {})) t.truthy(error.message) t.assert(error.message.includes('simulated error')) + nock.removeInterceptor(interceptor) }) +test('should support retries on error', async t => { + const client = new Client({ api_key: 'secret', apihost: 'test_host', proxy: '', retry: { retries: 2 } }) + const METHOD = 'GET' + const PATH = '/some/path' + + const interceptor = nock('https://test_host') + .get(PATH).times(2).replyWithError('simulated error') + .get(PATH).times(1).reply(200, 'now all good') Review comment: Is there a way to confirm the retry actually was called with another test assertion (passing a function to `retry` for a callback)? ########## File path: test/unit/client.test.js ########## @@ -22,18 +22,60 @@ const Client = require('../../lib/client') const http = require('http') const nock = require('nock') -// Note: this has to come before any of the proxy tests, as they interfere +// Note: All client.request tests have to come before any of the proxy tests, as they interfere + +test('should return response', async t => { + const client = new Client({ api_key: 'secret', apihost: 'test_host', proxy: '' }) + const METHOD = 'GET' + const PATH = '/some/path' + + const interceptor = nock('https://test_host').get(PATH).times(1).reply(200, 'all good') + const result = await client.request(METHOD, PATH, {}) + t.is(result.toString(), 'all good') + nock.removeInterceptor(interceptor) +}) + test('should handle http request errors', async t => { const client = new Client({ api_key: 'secret', apihost: 'test_host', proxy: '' }) const METHOD = 'GET' const PATH = '/some/path' - nock('https://test_host').get(PATH).replyWithError('simulated error') + const interceptor = nock('https://test_host').get(PATH).times(1).replyWithError('simulated error') const error = await t.throwsAsync(client.request(METHOD, PATH, {})) t.truthy(error.message) t.assert(error.message.includes('simulated error')) + nock.removeInterceptor(interceptor) }) +test('should support retries on error', async t => { + const client = new Client({ api_key: 'secret', apihost: 'test_host', proxy: '', retry: { retries: 2 } }) + const METHOD = 'GET' + const PATH = '/some/path' + + const interceptor = nock('https://test_host') + .get(PATH).times(2).replyWithError('simulated error') + .get(PATH).times(1).reply(200, 'now all good') + const result = await client.request(METHOD, PATH, {}) + t.is(result.toString(), 'now all good') + nock.removeInterceptor(interceptor) +}) + +test('should handle errors even after retries', async t => { + const client = new Client({ api_key: 'secret', apihost: 'test_host', proxy: '', retry: { retries: 2 } }) + const METHOD = 'GET' + const PATH = '/some/path' + + const interceptor = nock('https://test_host') + .get(PATH).times(3).replyWithError('simulated error') + .get(PATH).times(1).reply(200, 'not enough retries to come here') + const error = await t.throwsAsync(client.request(METHOD, PATH, {})) + t.truthy(error.message) + t.assert(error.message.includes('simulated error')) + nock.removeInterceptor(interceptor) +}) + Review comment: +1 - as I was reading the tests I was thinking the same thing (confirm both presence and absence of the retry). -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
