BUG: test.cb('', async(t) => {}) throws wrong error notification [or test.serial.cb when awaited]

[izelnakri]

version: 3.1.0
Code below gives an irrevelant error, and test fails while it shouldnt:

Error: Do not return promises from tests declared via `test.cb(...)`, if you want to return a promise simply declare the test via `test(...)`

test.serial.cb(
  "$ memserver | and $ memserver helper | and $ memserver h | without arguments shows help screen",
  async t => {
    t.plan(3);

    console.log("before read");
    const jsonBuffer = await fs.readFile(`${CWD}/package.json`);
    const version = JSON.parse(jsonBuffer.toString()).version;
    console.log("after read"); // NOTE: THIS NEVER GETS RUN DUE TO BUG
    const expectedOutput = `[MemServer CLI v${version}] Usage: memserver <command (Default: help)>

memserver init | new                    # Sets up the initial memserver folder structure
memserver generate model [ModelName]    # Generates the initial files for a MemServer Model [alias: "memserver g model"]
memserver generate fixtures             # Outputs your initial MemServer state as pure javascript fixture files
memserver generate fixtures [ModelName] # Outputs your initial MemServer state for certain model as pure javascript fixture
memserver console                       # Starts a MemServer console in node.js [alias: "memserver c"]
memserver serve | server [outputFile]   # Builds an ES5 javascript bundle with all your memserver code continuosly on watch [alias: "memserver s"]
memserver build | rollup [outputFile]   # Builds an ES5 javascript bundle with all your memserver code
memserver version | v                   # Displays memserver version
`;

    shell(`node ${CWD}/cli.js`, (error, stdout) => {
      t.true(stdout.includes(expectedOutput));

      shell(`node ${CWD}/cli.js help`, (error, stdout) => {
         t.true(stdout.includes(expectedOutput));

         shell(`node ${CWD}/cli.js help`, (error, stdout) => {
           t.true(stdout.includes(expectedOutput));

           t.end();
         });
       });
    });
  }

When I turn await fs.readFile to fs.readFileSync() it works so this is definitely a bug!

I guess this is happening because async function()'s have return Promise() during runtime. Such error/return check makes async keyword impossible on test.cb() and test.serial.cb(), which should be allowed in my opinion and current check isn't backwards-compatible because this behavior used to be allowed previously.

[sindresorhus]

Such error/return check makes async keyword impossible on test.cb() and test.serial.cb(), which should be allowed in my opinion and current check isn't backwards-compatible because this behavior used to be allowed previously.

This is intentional. test.cb is a legacy API. There's almost always a better way. In your case, you should use normal test.serial() in combination with util.promisify on the shell calls.

For example, if a shell() call errors now, you'll not get a helpful message as you're not handling the error. If you promisfy the shell() calls, it will just work.

The last use for test.cb is events, where you can just use https://github.com/sindresorhus/p-event.

[izelnakri]

@sindresorhus @novemberborn There are rare cases where callback APIs are favorable, I think the example above is one of them.

If I promisify it, then error handling becomes worse with try/catch on testing error scenarios, becomes much worse at being able to test multiple error scenarios at once, there are also rare cases where this is better than having a test case for each error. By deprecating test.cb() or making it legacy you are also disallowing the usage of promise.catch(() => {}) because there is no t.done() on normal test()’s.

[novemberborn]

We won't be changing this behavior.

[izelnakri]

ok, can we remove the API(test.cb and test.serial.cb), or deprecate it with a clear deprecation message?

[novemberborn]

I'd say these are legacy APIs, not quite deprecated.

Perhaps we can change the message? Something like:

Use test.cb() for legacy callback APIs. When using promises or async functions, use test().

[izelnakri]

Yes thats more clear 👍 However I think it would be even better if we remove or deprecate the methods(test.cb and test.serial.cb) altogether since callback based legacy apis can be promisified, current cb() implementation is very limited without async/await and there would be less documentation, learning and testing surface.

[novemberborn]

Sure. I've opened #2386 and #2387 to track. Chime in if you're interested in contributing, thanks! 😄