Skip to content

Commit

Permalink
V2/better getter error (#596)
Browse files Browse the repository at this point in the history
* better getter errors

- more precise getter error test

- get rid of invalid `assert.throws(` and `assert.rejects(` usages

there was accidentally passed an expected error message in place of assertion message

- refactor better-errors to produce `PageObjectError`

- allow to bypass explicit `key` specification in getter

- update docs

* tests: update visitable test to avoid deep comparison
  • Loading branch information
ro0gr authored Mar 24, 2023
1 parent 20a4614 commit e9908c8
Show file tree
Hide file tree
Showing 12 changed files with 196 additions and 116 deletions.
53 changes: 36 additions & 17 deletions addon/src/-private/better-errors.js
Original file line number Diff line number Diff line change
Expand Up @@ -20,31 +20,50 @@ export function throwContextualError(node, filters, e) {
* @return {Ember.Error}
*/
export function throwBetterError(node, key, error, { selector } = {}) {
let path = [key];
let current;

let fullErrorMessage =
error instanceof Error ? error.message : error.toString();
let message = error instanceof Error ? error.message : error.toString();

for (current = node; current; current = Ceibo.parent(current)) {
path.unshift(Ceibo.meta(current).key);
const err = new PageObjectError(key, node, selector, message);
if (error instanceof Error && 'stack' in error) {
err.stack = error.stack;
}

path[0] = 'page';
console.error(err.toString());
throw err;
}

if (path.length > 0) {
fullErrorMessage += `\n\nPageObject: '${path.join('.')}'`;
export class PageObjectError extends Error {
constructor(label, node, selector, ...args) {
super(...args);

this.label = label;
this.node = node;
this.selector = selector;
}

if (typeof selector === 'string' && selector.trim().length > 0) {
fullErrorMessage = `${fullErrorMessage}\n Selector: '${selector}'`;
toString() {
let { message, label, node, selector } = this;
if (label) {
let path = buildPropertyNamesPath(label, node);
message = `${message}\n\nPageObject: '${path.join('.')}'`;
}

if (typeof selector === 'string' && selector.trim().length > 0) {
message = `${message}\n Selector: '${selector}'`;
}

return `Error: ${message}`;
}
}

const err = new Error(fullErrorMessage);
if (error instanceof Error && 'stack' in error) {
err.stack = error.stack;
function buildPropertyNamesPath(leafKey, node) {
let path = [leafKey];

let current;
for (current = node; current; current = Ceibo.parent(current)) {
path.unshift(Ceibo.meta(current).key);
}

console.error(err.message);
throw err;
path[0] = 'page';

return path;
}
6 changes: 3 additions & 3 deletions addon/src/extend/find-element-with-assert.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,9 +9,9 @@
* import { getter } from 'ember-cli-page-object/macros';
* import { findElementWithAssert } from 'ember-cli-page-object/extend';
*
* export default function isDisabled(selector) {
* return getter(function (pageObjectKey) {
* return findElementWithAssert(this, selector, { pageObjectKey }).is(':disabled');
* export default function isDisabled(selector, options) {
* return getter(function () {
* return findElementWithAssert(this, selector, options).is(':disabled');
* });
* }
*
Expand Down
6 changes: 3 additions & 3 deletions addon/src/extend/find-element.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,9 +8,9 @@
* import { getter } from 'ember-cli-page-object/macros';
* import { findElement } from 'ember-cli-page-object/extend';
*
* function isDisabled(selector) {
* return getter(function (pageObjectKey) {
* return findElement(this, selector, { pageObjectKey }).is(':disabled');
* function isDisabled(selector, options = {}) {
* return getter(function () {
* return findElement(this, selector, options).is(':disabled');
* });
* }
*
Expand Down
4 changes: 2 additions & 2 deletions addon/src/extend/find-many.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,8 @@
* import { findMany } from 'ember-cli-page-object/extend';
*
* export default function count(selector, options = {}) {
* return getter(function (pageObjectKey) {
* return findMany(this, selector, { pageObjectKey }).length;
* return getter(function () {
* return findMany(this, selector, options).length;
* });
* }
*
Expand Down
6 changes: 3 additions & 3 deletions addon/src/extend/find-one.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,9 +8,9 @@
* import { getter } from 'ember-cli-page-object/macros';
* import { findOne } from 'ember-cli-page-object';
*
* function isDisabled(selector) {
* return getter(function (pageObjectKey) {
* return findOne(this, selector, { pageObjectKey }).disabled;
* function isDisabled(selector, options = {}) {
* return getter(function () {
* return findOne(this, selector, options).disabled;
* });
* }
*
Expand Down
16 changes: 14 additions & 2 deletions addon/src/macros/getter.js
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { throwBetterError } from '../-private/better-errors';
import { PageObjectError, throwBetterError } from '../-private/better-errors';

const NOT_A_FUNCTION_ERROR = 'Argument passed to `getter` must be a function.';

Expand Down Expand Up @@ -47,7 +47,19 @@ export function getter(fn) {
throwBetterError(this, pageObjectKey, NOT_A_FUNCTION_ERROR);
}

return fn.call(this, pageObjectKey);
try {
return fn.call(this, pageObjectKey);
} catch (e) {
if (e instanceof PageObjectError) {
if (!e.label) {
e.label = pageObjectKey;
}

throw e;
}

throwBetterError(this, pageObjectKey, e);
}
},
};
}
129 changes: 71 additions & 58 deletions test-app/tests/unit/-private/action-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,20 @@ import Adapter from 'ember-cli-page-object/adapter';

class DummyAdapter extends Adapter {}

function assertThrowsErrorMessage(
assert,
block,
expectedMessage,
asserionMessage
) {
try {
block();
assert.true(false);
} catch (e) {
assert.strictEqual(e.toString(), expectedMessage, asserionMessage);
}
}

// Normally it should work with "0".
// However, since Promises are not supported in IE11, for async we rely on "RSVP" library,
// Unfortunatelly, for some of old "ember" versions there are timing lags coming from "RSVP",
Expand Down Expand Up @@ -93,12 +107,13 @@ module('Unit | action', function (hooks) {
}),
});

assert.throws(
assertThrowsErrorMessage(
assert,
() => p.run(1),
new Error(`it was so fast!
`Error: it was so fast!
PageObject: 'page.run("1")'
Selector: '.Scope .Selector'`)
Selector: '.Scope .Selector'`
);
});

Expand All @@ -111,42 +126,40 @@ PageObject: 'page.run("1")'
}),
});

assert.throws(
assertThrowsErrorMessage(
assert,
() => p.run(1),
new Error(`it was so fast!
`Error: it was so fast!
PageObject: 'page.run("1")'
Selector: '.Scope'`)
Selector: '.Scope'`
);
});

test('it handles async errors', async function (assert) {
// when test against some old `ember-cli-qunit`-only scenarios, QUnit@1 is installed.
// There is no `assert.rejects(` support, so we just ignore the test in such cases.
// It's still tested on newer scenarios with QUnit@2, even against old "ember-test-helpers" scenarios.
//
// @todo: remove after `ember-cli-qunit` support is removed.
if (typeof assert.rejects === 'function') {
const p = create({
scope: '.Scope',
const p = create({
scope: '.Scope',

run: action({ selector: '.Selector' }, function () {
return next().then(() => {
throw new Error('bed time');
});
}),
});
run: action({ selector: '.Selector' }, function () {
return next().then(() => {
throw new Error('bed time');
});
}),
});

assert.rejects(
p.run(1),
new Error(`bed time
return p.run(1).then(
() => {
assert.true(false);
},
(e) => {
assert.strictEqual(
e.toString(),
`Error: bed time
PageObject: 'page.run("1")'
Selector: '.Scope .Selector'`)
);
} else {
assert.expect(0);
}
PageObject: 'page.run("1")'\n Selector: '.Scope .Selector'`
);
}
);
});

module('chainability', function () {
Expand Down Expand Up @@ -306,38 +319,38 @@ PageObject: 'page.run("1")'
});

test('it handles errors', async function (assert) {
// when test against some old `ember-cli-qunit`-only scenarios, QUnit@1 is installed.
// There is no `assert.rejects(` support, so we just ignore the test in such cases.
// It's still tested on newer scenarios with QUnit@2, even against old "ember-test-helpers" scenarios.
//
// @todo: remove after `ember-cli-qunit` support is removed.
if (typeof assert.rejects === 'function') {
const p = create({
scope: '.root',

emptyRun: action({ selector: '.Selector1' }, () => {}),

child: {
scope: '.child',

run: action({ selector: '.Selector2' }, function () {
return next().then(() => {
throw new Error('bed time');
});
}),
},
});
const p = create({
scope: '.root',

assert.rejects(
p.emptyRun().child.run(1),
new Error(`bed time
emptyRun: action({ selector: '.Selector1' }, () => {}),

PageObject: 'page.child.run("1")'
Selector: '.root .child .Selector2'`)
child: {
scope: '.child',

run: action({ selector: '.Selector2' }, function () {
return next().then(() => {
throw new Error('bed time');
});
}),
},
});

return p
.emptyRun()
.child.run(1)
.then(
() => {
assert.true(false);
},
(e) => {
assert.strictEqual(
e.toString(),
`Error: bed time
PageObject: 'page.child.run("1")'\n Selector: '.root .child .Selector2'`
);
}
);
} else {
assert.expect(0);
}
});
});
});
10 changes: 5 additions & 5 deletions test-app/tests/unit/-private/better-errors-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -19,8 +19,8 @@ module('Unit | throwBetterError', function () {
const fn = () => {
throwBetterError(page.foo.bar, 'focus', 'Oops!');
};
const expectedError = new Error(
"Oops!\n\nPageObject: 'page.foo.bar.focus'"
const expectedError = new RegExp(
`Oops!\n\nPageObject: 'page.foo.bar.focus'`
);

assert.throws(fn, expectedError, 'should show message & property path');
Expand All @@ -37,8 +37,8 @@ module('Unit | throwBetterError', function () {
});
} catch (e) {
assert.equal(
e.message,
"Oops!\n\nPageObject: 'page.foo.bar.focus'\n Selector: '.foo .bar'"
e.toString(),
"Error: Oops!\n\nPageObject: 'page.foo.bar.focus'\n Selector: '.foo .bar'"
);
assert.equal(e.stack, sourceError.stack, 'stack is preserved');
}
Expand All @@ -51,7 +51,7 @@ module('Unit | throwBetterError', function () {

try {
console.error = (msg) => {
assert.equal(msg, "Oops!\n\nPageObject: 'page.foo.bar.focus'");
assert.equal(msg, "Error: Oops!\n\nPageObject: 'page.foo.bar.focus'");
};

assert.throws(() => throwBetterError(page.foo.bar, 'focus', 'Oops!'));
Expand Down
11 changes: 7 additions & 4 deletions test-app/tests/unit/-private/composition-test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -153,9 +153,12 @@ module('Unit | composition', function () {
});

test('getPageObjectDefinition errors if node is not a page object', function (assert) {
assert.throws(function () {
return getPageObjectDefinition({});
}, 'cannot get the page object definition from a node that is not a page object');
try {
getPageObjectDefinition({});
assert.true(false);
} catch (e) {
assert.strictEqual(e.toString(), 'Error: cannot get the page object definition from a node that is not a page object');
}
});

module('all properties via compsition', function (hooks) {
Expand All @@ -174,7 +177,7 @@ module('Unit | composition', function () {
aliasPage: aliasPage,
});
await render(
hbs`<div class="container"><button>Look at me</button></div>`
hbs`<div class="container"><button type="button">Look at me</button></div>`
);

assert.ok(page.aliasPage.aliasedIsButtonVisible);
Expand Down
Loading

0 comments on commit e9908c8

Please sign in to comment.