Merge pull request #624 from ro0gr/early-callback-validation

fail fast if no callback passed
san650 · Oct 15, 2023 · cb09606 · cb09606
2 parents aab55b0 + eda2a70
commit cb09606
Show file tree

Hide file tree

Showing 4 changed files with 49 additions and 34 deletions.
diff --git a/addon/src/-private/action.js b/addon/src/-private/action.js
@@ -2,14 +2,20 @@ import { getter } from '../macros/index';
 import { run } from './run';
 import { throwContextualError } from './better-errors';
 
-export default function action(options, cb) {
+export default function action(options, act) {
+  [act, options] = normalizeArgs(options, act);
+
+  if (typeof act !== 'function') {
+    throw new Error('`action()` expects a function argument.');
+  }
+
   return getter(function (key) {
     return function (...args) {
-      ({ options, cb } = normalizeArgs(key, options, cb, args));
+      options.pageObjectKey = formatKey(key, args);
 
       return run(this, () => {
         try {
-          const invocation = cb.bind(this)(...args);
+          const invocation = act.bind(this)(...args);
 
           return Promise.resolve(invocation).catch((e) => {
             throwContextualError(this, options, e);
@@ -22,22 +28,16 @@ export default function action(options, cb) {
   });
 }
 
-function normalizeArgs(key, options, cb, args) {
-  const formattedKey = `${key}(${
-    args.length ? `"${args.map((a) => String(a)).join('", "')}"` : ``
-  })`;
-
-  if (typeof options === 'function') {
-    cb = options;
-    options = {
-      pageObjectKey: formattedKey,
-    };
-  } else {
-    options = {
-      ...options,
-      pageObjectKey: formattedKey,
-    };
+function normalizeArgs(options, act) {
+  if (!act) {
+    return [options, {}];
   }
 
-  return { options, cb };
+  return [act, { ...options }];
+}
+
+function formatKey(key, args) {
+  return `${key}(${
+    args.length ? `"${args.map((a) => String(a)).join('", "')}"` : ``
+  })`;
 }
diff --git a/addon/src/macros/getter.js b/addon/src/macros/getter.js
@@ -1,7 +1,5 @@
 import { PageObjectError, throwBetterError } from '../-private/better-errors';
 
-const NOT_A_FUNCTION_ERROR = 'Argument passed to `getter` must be a function.';
-
 /**
  * Creates a Descriptor whose value is determined by the passed-in function.
  * The passed-in function must not be bound and must not be an arrow function,
@@ -39,14 +37,14 @@ const NOT_A_FUNCTION_ERROR = 'Argument passed to `getter` must be a function.';
  * @throws Will throw an error if a function is not passed in as the first argument
  */
 export function getter(fn) {
+  if (typeof fn !== 'function') {
+    throw new Error('Argument passed to `getter` must be a function.');
+  }
+
   return {
     isDescriptor: true,
 
     get(pageObjectKey) {
-      if (typeof fn !== 'function') {
-        throwBetterError(this, pageObjectKey, NOT_A_FUNCTION_ERROR);
-      }
-
       try {
         return fn.call(this, pageObjectKey);
       } catch (e) {

diff --git a/test-app/tests/unit/-private/action-test.js b/test-app/tests/unit/-private/action-test.js
@@ -98,6 +98,30 @@ module('Unit | action', function (hooks) {
     assert.deepEqual(finished, [1]);
   });
 
+  test('fails without arguments', async function (assert) {
+    try {
+      action();
+      assert.false(true, 'throws an exception for no argument');
+    } catch (e) {
+      assert.strictEqual(
+        e?.toString(),
+        'Error: `action()` expects a function argument.'
+      );
+    }
+  });
+
+  test('fails without an action callback', async function (assert) {
+    try {
+      action({});
+      assert.false(true, 'throws an exception for a single object argument');
+    } catch (e) {
+      assert.strictEqual(
+        e?.toString(),
+        'Error: `action()` expects a function argument.'
+      );
+    }
+  });
+
   test('it handles sync errors', async function (assert) {
     const p = create({
       scope: '.Scope',

diff --git a/test-app/tests/unit/-private/properties/getter-test.ts b/test-app/tests/unit/-private/properties/getter-test.ts
@@ -71,17 +71,10 @@ module('getter', function(hooks) {
   test('throws an error if a function is not passed in', function(this: TestContext, assert) {
     assert.expect(1);
 
-    const page = create({
-      foo: getter('not a function' as any)
-    });
-
     try {
-      page.foo;
-      assert.true(false);
+      getter('not a function' as any);
     } catch (e) {
-      assert.strictEqual(e?.toString(), `Error: Argument passed to \`getter\` must be a function.
-
-PageObject: \'page.foo\'`)
+      assert.strictEqual(e?.toString(), `Error: Argument passed to \`getter\` must be a function.`)
     }
   });