From be53268af8ed0272791a0caf1d7b3ace1d8664ba Mon Sep 17 00:00:00 2001 From: Erwin Mombay Date: Thu, 8 Dec 2016 16:44:59 -0800 Subject: [PATCH 1/4] add expectedError method --- spec/amp-errors.md | 2 ++ src/log.js | 15 +++++++++++++++ src/service/storage-impl.js | 6 ++---- 3 files changed, 19 insertions(+), 4 deletions(-) diff --git a/spec/amp-errors.md b/spec/amp-errors.md index 8c8dd9ecbd04..5faea1b01101 100644 --- a/spec/amp-errors.md +++ b/spec/amp-errors.md @@ -45,6 +45,8 @@ The following fields are reported: The "expected" errors are marked with `&ex=1`. An expected error is identified by the `.expected` property on the `Error` object being `true`. +You can use the `Log.prototype.expectedError` method to create an error that is marked +as expected. An "expected" error is still an error, i.e. some features are disabled or not functioning fully because of it. However, it's an expected error. E.g. as is the diff --git a/src/log.js b/src/log.js index 796dffadfd78..08af32f12748 100644 --- a/src/log.js +++ b/src/log.js @@ -179,6 +179,7 @@ export class Log { * asynchronously. * @param {string} tag * @param {...*} var_args + * @return {!Error|undefined} */ error(tag, var_args) { if (this.level_ >= LogLevel.ERROR) { @@ -188,6 +189,20 @@ export class Log { Array.prototype.slice.call(arguments, 1)); this.prepareError_(error); this.win.setTimeout(() => {throw error;}); + return error; + } + } + + /** + * Reports an error message and marks with an expected property. If the + * logging is disabled, the error is rethrown asynchronously. + * @param {string} tag + * @param {...*} var_args + */ + expectedError(tag, var_args) { + const error = this.error(tag, var_args); + if (error) { + error.expected = true; } } diff --git a/src/service/storage-impl.js b/src/service/storage-impl.js index cd1f1a2714bb..e546fb27358a 100644 --- a/src/service/storage-impl.js +++ b/src/service/storage-impl.js @@ -111,7 +111,7 @@ export class Storage { this.storePromise_ = this.binding_.loadBlob(this.origin_) .then(blob => blob ? JSON.parse(atob(blob)) : {}) .catch(reason => { - dev().error(TAG, 'Failed to load store: ', reason); + dev().expectedError(TAG, 'Failed to load store: ', reason); return {}; }) .then(obj => new Store(obj)); @@ -289,9 +289,7 @@ export class LocalStorageBinding { this.isLocalStorageSupported_ = 'localStorage' in this.win; if (!this.isLocalStorageSupported_) { - const error = new Error('localStorage not supported.'); - error.expected = true; - dev().error(TAG, error); + dev().expectedError(TAG, 'localStorage not supported.'); } } From 51e3526713681b434e59269fc1a620128d2b09fc Mon Sep 17 00:00:00 2001 From: Erwin Mombay Date: Fri, 9 Dec 2016 11:07:28 -0800 Subject: [PATCH 2/4] add private helper method and tests --- src/log.js | 19 ++++++++++++++++--- test/functional/test-log.js | 24 ++++++++++++++++++++++++ 2 files changed, 40 insertions(+), 3 deletions(-) diff --git a/src/log.js b/src/log.js index 08af32f12748..d7f07e9ccb47 100644 --- a/src/log.js +++ b/src/log.js @@ -181,18 +181,30 @@ export class Log { * @param {...*} var_args * @return {!Error|undefined} */ - error(tag, var_args) { + error_(tag, var_args) { if (this.level_ >= LogLevel.ERROR) { this.msg_(tag, 'ERROR', Array.prototype.slice.call(arguments, 1)); } else { const error = createErrorVargs.apply(null, Array.prototype.slice.call(arguments, 1)); this.prepareError_(error); - this.win.setTimeout(() => {throw error;}); return error; } } + /** + * Reports an error message. + * @param {string} tag + * @param {...*} var_args + * @return {!Error|undefined} + */ + error(tag, var_args) { + const error = this.error_.apply(this, arguments); + if (error) { + this.win.setTimeout(() => {throw error;}); + } + } + /** * Reports an error message and marks with an expected property. If the * logging is disabled, the error is rethrown asynchronously. @@ -200,9 +212,10 @@ export class Log { * @param {...*} var_args */ expectedError(tag, var_args) { - const error = this.error(tag, var_args); + const error = this.error_.apply(this, arguments); if (error) { error.expected = true; + this.win.setTimeout(() => {throw error;}); } } diff --git a/test/functional/test-log.js b/test/functional/test-log.js index 6d42625fa8b6..4daab430d3b5 100644 --- a/test/functional/test-log.js +++ b/test/functional/test-log.js @@ -178,10 +178,34 @@ describe('Logging', () => { } catch (e) { expect(e).to.be.instanceof(Error); expect(e.message).to.match(/intended\: test/); + expect(e.expected).to.be.undefined; expect(isUserErrorMessage(e.message)).to.be.false; } }); + it('should report ERROR and mark with expected flag', () => { + const log = new Log(win, RETURNS_OFF); + expect(log.level_).to.equal(LogLevel.OFF); + win.setTimeout = () => {}; + let timeoutCallback; + const timeoutStub = sandbox.stub(win, 'setTimeout', callback => { + timeoutCallback = callback; + }); + + log.expectedError('TAG', 'intended', new Error('test')); + + expect(logSpy.callCount).to.equal(0); + expect(timeoutStub.callCount).to.equal(1); + expect(timeoutCallback).to.exist; + try { + timeoutCallback(); + } catch (e) { + expect(e).to.be.instanceof(Error); + expect(e.message).to.match(/intended\: test/); + expect(e.expected).to.be.true; + } + }); + it('should report ERROR when OFF from a single message', () => { const log = new Log(win, RETURNS_OFF); expect(log.level_).to.equal(LogLevel.OFF); From dfaf29aa6796b751e740def87b0d73ab3fd528dd Mon Sep 17 00:00:00 2001 From: Erwin Mombay Date: Fri, 9 Dec 2016 12:06:31 -0800 Subject: [PATCH 3/4] add private annotation --- src/log.js | 1 + 1 file changed, 1 insertion(+) diff --git a/src/log.js b/src/log.js index d7f07e9ccb47..71980cb428ba 100644 --- a/src/log.js +++ b/src/log.js @@ -180,6 +180,7 @@ export class Log { * @param {string} tag * @param {...*} var_args * @return {!Error|undefined} + * @private */ error_(tag, var_args) { if (this.level_ >= LogLevel.ERROR) { From d1f3715a7d85dc67b086ac891d8d1ef20383eba8 Mon Sep 17 00:00:00 2001 From: Erwin Mombay Date: Thu, 15 Dec 2016 11:08:54 -0800 Subject: [PATCH 4/4] add createExpectedError method and fix xhr-impl --- src/log.js | 16 ++++++++++++++-- src/service/storage-impl.js | 3 ++- src/service/viewer-impl.js | 2 +- src/service/xhr-impl.js | 8 ++++---- test/functional/test-log.js | 7 +++++++ test/functional/test-storage.js | 3 +-- test/functional/test-viewer.js | 6 ++++-- 7 files changed, 33 insertions(+), 12 deletions(-) diff --git a/src/log.js b/src/log.js index 71980cb428ba..0795f002a190 100644 --- a/src/log.js +++ b/src/log.js @@ -202,7 +202,7 @@ export class Log { error(tag, var_args) { const error = this.error_.apply(this, arguments); if (error) { - this.win.setTimeout(() => {throw error;}); + this.win.setTimeout(() => {throw /** @type {!Error} */ (error);}); } } @@ -216,7 +216,7 @@ export class Log { const error = this.error_.apply(this, arguments); if (error) { error.expected = true; - this.win.setTimeout(() => {throw error;}); + this.win.setTimeout(() => {throw /** @type {!Error} */ (error);}); } } @@ -231,6 +231,18 @@ export class Log { return error; } + /** + * Creates an error object with its expected property set to true. + * @param {...*} var_args + * @return {!Error} + */ + createExpectedError(var_args) { + const error = createErrorVargs.apply(null, arguments); + this.prepareError_(error); + error.expected = true; + return error; + } + /** * Throws an error if the first argument isn't trueish. * diff --git a/src/service/storage-impl.js b/src/service/storage-impl.js index e546fb27358a..d227b1aa40eb 100644 --- a/src/service/storage-impl.js +++ b/src/service/storage-impl.js @@ -289,7 +289,8 @@ export class LocalStorageBinding { this.isLocalStorageSupported_ = 'localStorage' in this.win; if (!this.isLocalStorageSupported_) { - dev().expectedError(TAG, 'localStorage not supported.'); + const error = new Error('localStorage not supported.'); + dev().expectedError(TAG, error); } } diff --git a/src/service/viewer-impl.js b/src/service/viewer-impl.js index 0c4f637c399b..661e0a7b73ff 100644 --- a/src/service/viewer-impl.js +++ b/src/service/viewer-impl.js @@ -334,7 +334,7 @@ export class Viewer { } else { resolve(this.win.document.referrer); if (this.unconfirmedReferrerUrl_ != this.win.document.referrer) { - dev().error(TAG_, 'Untrusted viewer referrer override: ' + + dev().expectedError(TAG_, 'Untrusted viewer referrer override: ' + this.unconfirmedReferrerUrl_ + ' at ' + this.messagingOrigin_); this.unconfirmedReferrerUrl_ = this.win.document.referrer; diff --git a/src/service/xhr-impl.js b/src/service/xhr-impl.js index a34c47007cf6..68985427363e 100644 --- a/src/service/xhr-impl.js +++ b/src/service/xhr-impl.js @@ -353,7 +353,7 @@ export function fetchPolyfill(input, opt_init) { } if (xhr.status < 100 || xhr.status > 599) { xhr.onreadystatechange = null; - reject(new Error(`Unknown HTTP status ${xhr.status}`)); + reject(user().createExpectedError(`Unknown HTTP status ${xhr.status}`)); return; } @@ -365,10 +365,10 @@ export function fetchPolyfill(input, opt_init) { } }; xhr.onerror = () => { - reject(new Error('Network failure')); + reject(user().createExpectedError('Network failure')); }; xhr.onabort = () => { - reject(new Error('Request aborted')); + reject(user().createExpectedError('Request aborted')); }; if (init.method == 'POST') { @@ -394,7 +394,7 @@ function createXhrRequest(method, url) { xhr = new XDomainRequest(); xhr.open(method, url); } else { - throw new Error('CORS is not supported'); + throw dev().createExpectedError('CORS is not supported'); } return xhr; } diff --git a/test/functional/test-log.js b/test/functional/test-log.js index 4daab430d3b5..bf67a755e5d3 100644 --- a/test/functional/test-log.js +++ b/test/functional/test-log.js @@ -406,8 +406,15 @@ describe('Logging', () => { expect(false).to.be.true; }); + it('should create expected error from message', () => { + const error = log.createExpectedError('test'); + expect(error).to.be.instanceof(Error); + expect(error.expected).to.be.true; + }); + it('should create suffixed errors from message', () => { const error = log.createError('test'); + expect(error.expected).to.be.undefined; expect(error).to.be.instanceof(Error); expect(isUserErrorMessage(error.message)).to.be.true; expect(error.message).to.contain('test'); diff --git a/test/functional/test-storage.js b/test/functional/test-storage.js index b209ebbfb884..ee073020497f 100644 --- a/test/functional/test-storage.js +++ b/test/functional/test-storage.js @@ -446,7 +446,7 @@ describe('LocalStorageBinding', () => { }); it('should throw if localStorage is not supported', () => { - const errorSpy = sandbox.spy(dev(), 'error'); + const errorSpy = sandbox.spy(dev(), 'expectedError'); expect(errorSpy.callCount).to.equal(0); new LocalStorageBinding(windowApi); @@ -456,7 +456,6 @@ describe('LocalStorageBinding', () => { new LocalStorageBinding(windowApi); expect(errorSpy.callCount).to.equal(1); expect(errorSpy.args[0][1].message).to.match(/localStorage not supported/); - expect(errorSpy.args[0][1].expected).to.be.true; }); it('should load store when available', () => { diff --git a/test/functional/test-viewer.js b/test/functional/test-viewer.js index abaf7ec7e13f..279e0d3e5ebd 100644 --- a/test/functional/test-viewer.js +++ b/test/functional/test-viewer.js @@ -33,6 +33,7 @@ describe('Viewer', () => { let clock; let events; let errorStub; + let expectedErrorStub; function changeVisibility(vis) { windowApi.document.hidden = vis !== 'visible'; @@ -83,6 +84,7 @@ describe('Viewer', () => { installTimerService(windowApi); events = {}; errorStub = sandbox.stub(dev(), 'error'); + expectedErrorStub = sandbox.stub(dev(), 'expectedError'); windowMock = sandbox.mock(windowApi); viewer = new Viewer(ampdoc); }); @@ -1051,8 +1053,8 @@ describe('Viewer', () => { // Unconfirmed referrer is reset. Async error is thrown. expect(viewer.getUnconfirmedReferrerUrl()) .to.equal('https://acme.org/docref'); - expect(errorStub.callCount).to.equal(1); - expect(errorStub.calledWith('Viewer', + expect(expectedErrorStub.callCount).to.equal(1); + expect(expectedErrorStub.calledWith('Viewer', sinon.match(arg => { return !!arg.match(/Untrusted viewer referrer override/); }))).to.be.true;