Async Function are not being tested like `runAuthScript`

[priyank-p]

Mocha by default cannot test async function it will mark it passed if the function does not throw Error of any sort synchronously.

I am working on fix. I just need to know all the async functions.

[priyank-p]

Only file needed to get update is auth.test.js which is bit tricky as it does not return promise but will asynchronously throw error.

[joyeecheung]

@cPhost Good catch, I think you will only need to mark the function passed to it async, return a Promise from runAuthScript that resolves/rejects in proc.on('close')/whereever there is an assert? cc @addaleax

[refack]

Good catch, I think you will only need to mark the function passed to it async, return a Promise from runAuthScript that resolves in proc.on('close')?

I requires a little bit more work, need to replace all the throws with the wrapping promise's reject, and find the place that signifies the end of the test and call resolve. I couldn't get it to work since after that's done it still doesn't work on Windows (probably because of the explicit \n that in some places might need to be replaced with os.EOL), and I didn't find the place where the promise should be resolved.

[addaleax]

@joyeecheung Yeah, basically that’s it (@refack I think Joyee had ~ the same thing in mind)

[priyank-p]

@joyeecheung @refack the current problem is when proc.on('close') is wrapped in a Promise mocha times out as the event is emitted at the very end when all the tests are done!' Also tried async no luck! also tried this.timeout(30000) still causes test to fail

[joyeecheung]

@cPhost You can hold off the resolve in proc.on('close') so Mocha will wait for it. This is what I roughly had in mind..(not pretty, needs polishing)

diff --git a/test/unit/auth.test.js b/test/unit/auth.test.js
index 8436b30..40ad742 100644
--- a/test/unit/auth.test.js
+++ b/test/unit/auth.test.js
@@ -8,9 +8,10 @@ const fs = require('fs');
 const assert = require('assert');
 let testCounter = 0; // for tmp directories

-describe('auth', () => {
-  it('asks for auth data if no ncurc is found', () => {
-    runAuthScript(undefined, [
+describe('auth', async function() {
+  it('asks for auth data if no ncurc is found', async function() {
+    this.timeout(1000);
+    await runAuthScript(undefined, [
       'Reading configuration for node-core-utils failed:',
       /ENOENT: no such file or directory, open/,
       'Please enter your Github user information:',
@@ -21,8 +22,9 @@ describe('auth', () => {
     ]);
   });

-  it('asks for auth data if ncurc is invalid json', () => {
-    runAuthScript('this is not json', [
+  it('asks for auth data if ncurc is invalid json', async function() {
+    this.timeout(1000);
+    await runAuthScript('this is not json', [
       'Reading configuration for node-core-utils failed:',
       /Unexpected token h in JSON at position 1/,
       'Please enter your Github user information:',
@@ -33,80 +35,98 @@ describe('auth', () => {
     ]);
   });

-  it('returns ncurc data if it is present and valid', () => {
-    runAuthScript({ username: 'nyancat', token: '0123456789abcdef' }, [
+  it('returns ncurc data if it is present and valid', async function() {
+    this.timeout(1000);
+    await runAuthScript({ username: 'nyancat', token: '0123456789abcdef' }, [
       'bnlhbmNhdDowMTIzNDU2Nzg5YWJjZGVm'
     ]);
   });
 });

 function runAuthScript(ncurc = undefined, expect = []) {
-  const HOME = path.resolve(__dirname, `tmp-${testCounter++}`);
-  rimraf.sync(HOME);
-  mkdirp.sync(HOME);
-  const ncurcPath = path.resolve(HOME, '.ncurc');
+  return new Promise((resolve, reject) => {
+    const HOME = path.resolve(__dirname, `tmp-${testCounter++}`);
+    rimraf.sync(HOME);
+    mkdirp.sync(HOME);
+    const ncurcPath = path.resolve(HOME, '.ncurc');

-  if (ncurc !== undefined) {
-    if (typeof ncurc === 'string') {
-      fs.writeFileSync(ncurcPath, ncurc, 'utf8');
-    } else {
-      fs.writeFileSync(ncurcPath, JSON.stringify(ncurc), 'utf8');
+    if (ncurc !== undefined) {
+      if (typeof ncurc === 'string') {
+        fs.writeFileSync(ncurcPath, ncurc, 'utf8');
+      } else {
+        fs.writeFileSync(ncurcPath, JSON.stringify(ncurc), 'utf8');
+      }
     }
-  }

-  const proc = spawn(process.execPath,
-    [ require.resolve('../fixtures/run-auth') ],
-    // XXX this could just be env: { ...process.env, HOME } but the test loader
-    // is complaining?
-    { env: Object.assign({}, process.env, { HOME }) });
-  let stderr = '';
-  proc.stderr.setEncoding('utf8');
-  proc.stderr.on('data', (chunk) => { stderr += chunk; });
-  proc.on('close', () => {
-    if (stderr) { throw new Error(`unexpected stderr:\n${stderr}`); }
-  });
+    const proc = spawn(process.execPath,
+      [ require.resolve('../fixtures/run-auth') ],
+      // XXX this could just be env: { ...process.env, HOME } but the test loader
+      // is complaining?
+      { timeout: 1000, env: Object.assign({}, process.env, { HOME }) });
+    let stderr = '';
+    proc.stderr.setEncoding('utf8');
+    proc.stderr.on('data', (chunk) => { stderr += chunk; });
+    proc.on('error', (err) => {
+      proc.kill();
+      reject(err);
+    });
+    proc.on('close', () => {
+      try {
+        assert.strictEqual(stderr, '');
+        assert.strictEqual(expect.length, 0);
+        rimraf.sync(HOME);
+      } catch (err) {
+        reject(err);
+      }
+      resolve();
+    });

-  let pendingStdout = '';
-  let flushNotYetTerminatedLineTimeout = null;
-  proc.stdout.on('data', (chunk) => {
-    pendingStdout += chunk;
-    clearTimeout(flushNotYetTerminatedLineTimeout);
-    flushNotYetTerminatedLineTimeout = null;
+    let pendingStdout = '';
+    let flushNotYetTerminatedLineTimeout = null;
+    proc.stdout.on('data', (chunk) => {
+      pendingStdout += chunk;
+      clearTimeout(flushNotYetTerminatedLineTimeout);
+      flushNotYetTerminatedLineTimeout = null;

-    let newlineIndex;
-    while ((newlineIndex = pendingStdout.indexOf('\n')) !== -1) {
-      const line = pendingStdout.substr(0, newlineIndex);
-      pendingStdout = pendingStdout.substr(newlineIndex + 1);
+      try {
+        let newlineIndex;
+        while ((newlineIndex = pendingStdout.indexOf('\n')) !== -1) {
+          const line = pendingStdout.substr(0, newlineIndex);
+          pendingStdout = pendingStdout.substr(newlineIndex + 1);

-      onLine(line);
-    }
+          onLine(line);
+        }

-    if (pendingStdout.length > 0) {
-      flushNotYetTerminatedLineTimeout = setTimeout(() => {
-        onLine(pendingStdout);
-        pendingStdout = '';
-      }, 100);
-    }
-  });
+        if (pendingStdout.length > 0) {
+          flushNotYetTerminatedLineTimeout = setTimeout(() => {
+            onLine(pendingStdout);
+            pendingStdout = '';
+          }, 100);
+        }
+      } catch (err) {
+        proc.kill();
+        reject(err);
+      }
+    });

-  function onLine(line) {
-    if (expect.length === 0) { throw new Error(`unexpected stdout line: ${line}`); }
-    let expected = expect.shift();
-    let reply;
-    if (typeof expected.reply === 'string') {
-      ({ expected, reply } = expected);
-    }
-    if (typeof expected === 'string') {
-      expected = new RegExp(`^${expected}$`);
-    }
-    assert(line.match(expected), `${line} should match ${expected}`);
-    if (reply !== undefined) {
-      proc.stdin.write(`${reply}\n`);
+    function onLine(line) {
+      assert.notStrictEqual(expect.length, 0, `unexpected stdout line: ${line}`);
+      let expected = expect.shift();
+      let reply;
+      if (typeof expected.reply === 'string') {
+        ({ expected, reply } = expected);
+      }
+      if (typeof expected === 'string') {
+        expected = new RegExp(`^${expected}$`);
+      }
+
+      assert(line.match(expected), `${line} should match ${expected}`);
+      if (reply !== undefined) {
+        proc.stdin.write(`${reply}\n`);
+      }
+      if (expect.length === 0) {
+        proc.stdin.end();
+      }
     }
-    if (expect.length === 0) { proc.stdin.end(); }
-  }
-  proc.on('close', () => {
-    assert.strictEqual(expect.length, 0);
-    rimraf.sync(HOME);
   });
 }

[priyank-p]

@joyeecheung does that work on mocha because i have pretty much the same thing for proc.on('close'!

[joyeecheung]

@cPhost Mocha works on async functions, so as long as you can make the promise resolve/reject the way you want it works. I've tested it with a few cases and it seems to fail/pass properly

[priyank-p]

@joyeecheung this is what i had. Looks ugly as i was trying diffrent thing

describe('child process', () => {
    it('child process did not have error at close event', async (done) => {
      let test = new Promise((reject, resolve) => {
        proc.on('close', () => {
          if(stderr) {
            reject(new Error(`unexpected stderr:\n${stderr}`));
          } else {
            resolve();
          }
        });
      });

      await test
      .then(_ => { done() })
      .catch(err => { 
        done(err);
      })
    });
  });

[joyeecheung]

@cPhost I think you don't have to mind the done part if it's already in async functions, see https://mochajs.org/#using-async--await

Also probably should not resolve right there unless you merge the two proc.on('close') listener, because the other one needs to run before resolving as well.

[priyank-p]

@joyeecheung i knew about it from before and so i tried both async and node style it(msg, function(done) {}).

But the problem was i was describing a testing on an event emmiter which timed out. I don't know what i was thinking!!!

But this issue got resolved quick.