From 04d2de96df4318a6537563b64d99dd975f11c8a7 Mon Sep 17 00:00:00 2001
From: Oliver Eyton-Williams <ojeytonwilliams@gmail.com>
Date: Mon, 4 Nov 2019 12:42:19 +0100
Subject: [PATCH] fix: conditionally log non-assertion errors (JS)

console.logs and errors are only reported during the first evaluation of
the user's code.  This is because the code is evaluated for each test,
but the logs will not change between the build phases of the tests.

Errors thrown during testing (except failing assertions) are always
reported. This is to inform the user that their code is faulty, rather
than that it does not meet the challenge's requirements.
---
 client/src/client/workers/test-evaluator.js   | 42 +++++++++++++------
 .../redux/execute-challenge-saga.js           | 12 +++++-
 .../src/templates/Challenges/utils/build.js   |  4 +-
 3 files changed, 41 insertions(+), 17 deletions(-)

diff --git a/client/src/client/workers/test-evaluator.js b/client/src/client/workers/test-evaluator.js
index 2a8a1eaec6..c13b3c3b79 100644
--- a/client/src/client/workers/test-evaluator.js
+++ b/client/src/client/workers/test-evaluator.js
@@ -24,22 +24,37 @@ const __utils = (() => {
   }
 
   const oldLog = self.console.log.bind(self.console);
-  self.console.log = function proxyConsole(...args) {
+  function proxyLog(...args) {
     logs.push(args.map(arg => '' + JSON.stringify(arg, replacer)).join(' '));
     if (logs.join('\n').length > MAX_LOGS_SIZE) {
       flushLogs();
     }
     return oldLog(...args);
-  };
+  }
 
+  // unless data.type is truthy, this sends data out to the testRunner
   function postResult(data) {
     flushLogs();
     self.postMessage(data);
   }
 
+  function logToBoth(err) {
+    if (!(err instanceof chai.AssertionError)) {
+      // report to both the browser and the fcc consoles, discarding the
+      // stack trace via toString as it only useful to debug the site, not a
+      // specific challenge.
+      console.log(err.toString());
+    }
+  }
+
+  const toggleLogging = on => {
+    self.console.log = on ? proxyLog : () => {};
+  };
+
   return {
     postResult,
-    oldLog
+    logToBoth,
+    toggleLogging
   };
 })();
 
@@ -50,6 +65,8 @@ self.onmessage = async e => {
   const assert = chai.assert;
   // Fake Deep Equal dependency
   const DeepEqual = (a, b) => JSON.stringify(a) === JSON.stringify(b);
+
+  __utils.toggleLogging(e.data.firstTest);
   /* eslint-enable no-unused-vars */
   try {
     let testResult;
@@ -65,14 +82,11 @@ self.onmessage = async e => {
       if (__userCodeWasExecuted) {
         // rethrow error, since test failed.
         throw err;
-      } else if (e.data.testString) {
-        // report errors to dev console if tests are running (since some
-        // challenges should pass with code that throws errors)
-        __utils.oldLog(err);
-      } else {
-        // user is editing code, so both consoles should report errors
-        console.log(err.toString());
       }
+      // log build errors
+      __utils.logToBoth(err);
+      // the tests may not require working code, so they are evaluated even if
+      // the user code does not get executed.
       testResult = eval(e.data.testString);
     }
     /* eslint-enable no-eval */
@@ -83,15 +97,17 @@ self.onmessage = async e => {
       pass: true
     });
   } catch (err) {
+    // report errors that happened during testing, so the user learns of
+    // execution errors and not just build errors.
+    __utils.toggleLogging(true);
+    __utils.logToBoth(err);
+    // postResult flushes the logs and must be called after logging is finished.
     __utils.postResult({
       err: {
         message: err.message,
         stack: err.stack
       }
     });
-    if (!(err instanceof chai.AssertionError)) {
-      console.error(err);
-    }
   }
 };
 
diff --git a/client/src/templates/Challenges/redux/execute-challenge-saga.js b/client/src/templates/Challenges/redux/execute-challenge-saga.js
index e4dbf9a771..a348776b48 100644
--- a/client/src/templates/Challenges/redux/execute-challenge-saga.js
+++ b/client/src/templates/Challenges/redux/execute-challenge-saga.js
@@ -92,10 +92,18 @@ function* buildChallengeData(challengeData) {
 
 function* executeTests(testRunner, tests, testTimeout = 5000) {
   const testResults = [];
-  for (const { text, testString } of tests) {
+  for (let i = 0; i < tests.length; i++) {
+    const { text, testString } = tests[i];
     const newTest = { text, testString };
+    // only the last test outputs console.logs to avoid log duplication.
+    const firstTest = i === 1;
     try {
-      const { pass, err } = yield call(testRunner, testString, testTimeout);
+      const { pass, err } = yield call(
+        testRunner,
+        testString,
+        testTimeout,
+        firstTest
+      );
       if (pass) {
         newTest.pass = true;
       } else {
diff --git a/client/src/templates/Challenges/utils/build.js b/client/src/templates/Challenges/utils/build.js
index 9598e7a1dc..10300ad72a 100644
--- a/client/src/templates/Challenges/utils/build.js
+++ b/client/src/templates/Challenges/utils/build.js
@@ -99,9 +99,9 @@ function getJSTestRunner({ build, sources }, proxyLogger) {
 
   const testWorker = createWorker(testEvaluator, { terminateWorker: true });
 
-  return (testString, testTimeout) => {
+  return (testString, testTimeout, firstTest = true) => {
     return testWorker
-      .execute({ build, testString, code, sources }, testTimeout)
+      .execute({ build, testString, code, sources, firstTest }, testTimeout)
       .on('LOG', proxyLogger).done;
   };
 }