From 7d0a4b01d33d4938c2562a32aca235f8c9f331c2 Mon Sep 17 00:00:00 2001 From: Ruslan Hrabovyi Date: Fri, 19 Jan 2024 02:44:27 +0200 Subject: [PATCH] fix: restore getter error details (#631) * fix: restore getter error details (#596) broke QUnit error messages so they now miss contextual error info like page object path and selector. This happened cause #596 relied on the `.toString()` method of the Error to build the final error message. However, that's not how QUnit is displaying the error message. It just uses the `Error.message`. So let's build the final message in the `Error` constructor. --- addon/src/-private/better-errors.js | 58 ++++++++++++------- addon/src/macros/getter.js | 13 ++++- .../comma-separated-selector-test.ts | 6 +- .../unit/-private/properties/getter-test.ts | 12 +++- test-app/tests/unit/extend/find-one-test.ts | 11 +++- 5 files changed, 69 insertions(+), 31 deletions(-) diff --git a/addon/src/-private/better-errors.js b/addon/src/-private/better-errors.js index a8b6adb6..9973d7d0 100644 --- a/addon/src/-private/better-errors.js +++ b/addon/src/-private/better-errors.js @@ -22,47 +22,63 @@ export function throwContextualError(node, filters, e) { export function throwBetterError(node, key, error, { selector } = {}) { let message = error instanceof Error ? error.message : error.toString(); - const err = new PageObjectError(key, node, selector, message); + const wrapperError = new PageObjectError(message, { + cause: { + message, + key, + node, + selector, + }, + }); + if (error instanceof Error && 'stack' in error) { - err.stack = error.stack; + wrapperError.stack = error.stack; } - console.error(err.toString()); - throw err; + console.error(wrapperError.toString()); + throw wrapperError; } export class PageObjectError extends Error { - constructor(label, node, selector, ...args) { - super(...args); + constructor(message, options = {}, ...args) { + const { cause } = options; + const { node, key, selector } = cause || {}; + + const errorDescription = buildErrorDescription(node, key, selector); - this.label = label; - this.node = node; - this.selector = selector; + super( + [message, errorDescription].filter(Boolean).join('\n'), + options, + ...args + ); } +} - toString() { - let { message, label, node, selector } = this; - if (label) { - let path = buildPropertyNamesPath(label, node); - message = `${message}\n\nPageObject: '${path.join('.')}'`; - } +function buildErrorDescription(node, key, selector) { + const lines = []; - if (typeof selector === 'string' && selector.trim().length > 0) { - message = `${message}\n Selector: '${selector}'`; - } + const path = buildPropertyNamesPath(node); + if (key) { + path.push(key); + } + lines.push(`\nPageObject: '${path.join('.')}'`); - return `Error: ${message}`; + if (typeof selector === 'string' && selector.trim().length > 0) { + lines.push(` Selector: '${selector}'`); } + + return lines.join('\n'); } -function buildPropertyNamesPath(leafKey, node) { - let path = [leafKey]; +function buildPropertyNamesPath(node) { + let path = []; let current; for (current = node; current; current = Ceibo.parent(current)) { path.unshift(Ceibo.meta(current).key); } + // replace "root" with "page" path[0] = 'page'; return path; diff --git a/addon/src/macros/getter.js b/addon/src/macros/getter.js index f8e669e0..87992902 100644 --- a/addon/src/macros/getter.js +++ b/addon/src/macros/getter.js @@ -49,8 +49,17 @@ export function getter(fn) { return fn.call(this, pageObjectKey); } catch (e) { if (e instanceof PageObjectError) { - if (!e.label) { - e.label = pageObjectKey; + if (!e.cause.key) { + // re-throw with a `pageObjectKey` to have a complete error message + const wrapperError = new PageObjectError(e.cause.message, { + cause: { + ...e.cause, + key: pageObjectKey, + }, + }); + wrapperError.stack = e.stack; + + throw wrapperError; } throw e; diff --git a/test-app/tests/integration/comma-separated-selector-test.ts b/test-app/tests/integration/comma-separated-selector-test.ts index 97887d18..fa288e0b 100644 --- a/test-app/tests/integration/comma-separated-selector-test.ts +++ b/test-app/tests/integration/comma-separated-selector-test.ts @@ -16,7 +16,7 @@ module('comma separated selectors', function (hooks) { assert.throws( () => page.isVisible, - new Error( + new RegExp( 'Usage of comma separated selectors is not supported. Please make sure your selector targets a single selector.' ) ); @@ -31,7 +31,7 @@ module('comma separated selectors', function (hooks) { assert.throws( () => page.text, - new Error( + new RegExp( 'Usage of comma separated selectors is not supported. Please make sure your selector targets a single selector.' ) ); @@ -50,7 +50,7 @@ module('comma separated selectors', function (hooks) { assert.throws( () => page.text, - new Error( + new RegExp( 'Usage of comma separated selectors is not supported. Please make sure your selector targets a single selector.' ) ); diff --git a/test-app/tests/unit/-private/properties/getter-test.ts b/test-app/tests/unit/-private/properties/getter-test.ts index bb9ca16b..31facc54 100644 --- a/test-app/tests/unit/-private/properties/getter-test.ts +++ b/test-app/tests/unit/-private/properties/getter-test.ts @@ -89,7 +89,7 @@ module('getter', function (hooks) { try { page.foo; - assert.true(false); + assert.false(true, 'should not succeed'); } catch (e) { assert.strictEqual( e?.toString(), @@ -109,6 +109,14 @@ PageObject: 'page.foo'` }), }); - assert.throws(() => page.foo, /Selector: '.non-existing-scope'/); + try { + page.foo; + assert.false(true, 'should not succeed'); + } catch (e: any) { + assert.strictEqual( + e.toString(), + `Error: Element not found.\n\nPageObject: 'page.foo'\n Selector: '.non-existing-scope'` + ); + } }); }); diff --git a/test-app/tests/unit/extend/find-one-test.ts b/test-app/tests/unit/extend/find-one-test.ts index 9eefaad0..443cec5c 100644 --- a/test-app/tests/unit/extend/find-one-test.ts +++ b/test-app/tests/unit/extend/find-one-test.ts @@ -40,13 +40,18 @@ module(`Extend | findOne`, function (hooks) { }); test('throws error if 0 elements found', async function (assert) { - const page = create({}); + const page = create({ + child: {}, + }); await render(hbs``); assert.throws( - () => findOne(page, '.unknown', {}), - /Error: Element not found./ + () => findOne(page.child, '.unknown', {}), + new Error(`Element not found. + +PageObject: 'page.child' + Selector: '.unknown'`) ); });