fix: remove JS comments revisited (#41952)

* fix: restrict removeJSComments to test-evaluator

To prevent @babel from being included everywhere, this only calls
removeJSComments inside the test-evaluator

* test: add removeJSComments test

client/src/client/workers/test-evaluator.js

@@ -2,7 +2,9 @@ import chai from 'chai';
 import '@babel/polyfill';
 import { toString as __toString } from 'lodash-es';
 import { format as __format } from '../../utils/format';
-import curriculumHelpers from '../../utils/curriculum-helpers';
+import curriculumHelpers, {
+  removeJSComments
+} from '../../utils/curriculum-helpers';
 
 const __utils = (() => {
   const MAX_LOGS_SIZE = 64 * 1024;
@@ -56,8 +58,12 @@ const __utils = (() => {
 /* Run the test if there is one.  If not just evaluate the user code */
 self.onmessage = async e => {
   /* eslint-disable no-unused-vars */
-  const code = (e.data?.code?.contents || '').slice();
-  const editableContents = (e.data?.code?.editableContents || '').slice();
+  let code = (e.data?.code?.contents || '').slice();
+  code = e.data?.removeComments ? removeJSComments(code) : code;
+  let editableContents = (e.data?.code?.editableContents || '').slice();
+  editableContents = e.data?.removeComments
+    ? removeJSComments(editableContents)
+    : editableContents;
 
   const assert = chai.assert;
   const __helpers = curriculumHelpers;
@@ -74,7 +80,9 @@ self.onmessage = async e => {
     try {
       // Logging is proxyed after the build to catch console.log messages
       // generated during testing.
-      testResult = eval(`${e.data.build}
+      testResult = eval(`${
+        e.data?.removeComments ? removeJSComments(e.data.build) : e.data.build
+      }
 __utils.flushLogs();
 __userCodeWasExecuted = true;
 __utils.toggleProxyLogger(true);

client/src/templates/Challenges/classic/Show.js

@@ -172,7 +172,7 @@ class ShowClassic extends Component {
     updateChallengeMeta({
       ...challengeMeta,
       title,
-      removeComments,
+      removeComments: removeComments !== false,
       challengeType,
       helpCategory
     });

client/src/templates/Challenges/redux/execute-challenge-saga.js

@@ -87,14 +87,13 @@ export function* executeChallengeSaga({
     const protect = isLoopProtected(challengeMeta);
     const buildData = yield buildChallengeData(challengeData, {
       preview: false,
-      removeComments: challengeMeta.removeComments,
       protect
     });
     const document = yield getContext('document');
     const testRunner = yield call(
       getTestRunner,
       buildData,
-      { proxyLogger },
+      { proxyLogger, removeComments: challengeMeta.removeComments },
       document
     );
     const testResults = yield executeTests(testRunner, tests);
@@ -202,7 +201,6 @@ function* previewChallengeSaga({ flushLogs = true } = {}) {
       const protect = isLoopProtected(challengeMeta);
       const buildData = yield buildChallengeData(challengeData, {
         preview: true,
-        removeComments: challengeMeta.removeComments,
         protect
       });
       // evaluate the user code in the preview frame or in the worker
@@ -210,7 +208,10 @@ function* previewChallengeSaga({ flushLogs = true } = {}) {
         const document = yield getContext('document');
         yield call(updatePreview, buildData, document, proxyLogger);
       } else if (isJavaScriptChallenge(challengeData)) {
-        const runUserCode = getTestRunner(buildData, { proxyLogger });
+        const runUserCode = getTestRunner(buildData, {
+          proxyLogger,
+          removeComments: challengeMeta.removeComments
+        });
         // without a testString the testRunner just evaluates the user's code
         yield call(runUserCode, null, previewTimeout);
       }

client/src/templates/Challenges/utils/build.js

@@ -13,7 +13,6 @@ import {
 import frameRunnerData from '../../../../../config/client/frame-runner.json';
 // eslint-disable-next-line import/no-unresolved
 import testEvaluatorData from '../../../../../config/client/test-evaluator.json';
-import { removeJSComments } from '../../../utils/curriculum-helpers';
 
 const { filename: runner } = frameRunnerData;
 const { filename: testEvaluator } = testEvaluatorData;
@@ -107,16 +106,16 @@ const testRunners = {
   [challengeTypes.backend]: getDOMTestRunner,
   [challengeTypes.pythonProject]: getDOMTestRunner
 };
-export function getTestRunner(buildData, { proxyLogger }, document) {
+export function getTestRunner(buildData, runnerConfig, document) {
   const { challengeType } = buildData;
   const testRunner = testRunners[challengeType];
   if (testRunner) {
-    return testRunner(buildData, proxyLogger, document);
+    return testRunner(buildData, runnerConfig, document);
   }
   throw new Error(`Cannot get test runner for challenge type ${challengeType}`);
 }
 
-function getJSTestRunner({ build, sources }, proxyLogger) {
+function getJSTestRunner({ build, sources }, { proxyLogger, removeComments }) {
   const code = {
     contents: sources.index,
     editableContents: sources.editableContents
@@ -126,12 +125,15 @@ function getJSTestRunner({ build, sources }, proxyLogger) {
 
   return (testString, testTimeout, firstTest = true) => {
     return testWorker
-      .execute({ build, testString, code, sources, firstTest }, testTimeout)
+      .execute(
+        { build, testString, code, sources, firstTest, removeComments },
+        testTimeout
+      )
       .on('LOG', proxyLogger).done;
   };
 }
 
-async function getDOMTestRunner(buildData, proxyLogger, document) {
+async function getDOMTestRunner(buildData, { proxyLogger }, document) {
   await new Promise(resolve =>
     createTestFramer(document, resolve, proxyLogger)(buildData)
   );
@@ -165,27 +167,16 @@ export function buildJSChallenge({ files }, options) {
     .map(pipeLine);
   return Promise.all(finalFiles)
     .then(checkFilesErrors)
-    .then(files => {
-      let build = files
+    .then(files => ({
+      challengeType: challengeTypes.js,
+      build: files
         .reduce(
           (body, file) => [...body, file.head, file.contents, file.tail],
           []
         )
-        .join('\n');
-      let sources = buildSourceMap(files);
-      if (options?.removeComments !== false) {
-        build = removeJSComments(build);
-        sources = {
-          ...sources,
-          index: removeJSComments(sources.index)
-        };
-      }
-      return {
-        challengeType: challengeTypes.js,
-        build,
-        sources
-      };
-    });
+        .join('\n'),
+      sources: buildSourceMap(files)
+    }));
 }
 
 export function buildBackendChallenge({ url }) {

client/src/utils/curriculum-helpers.test.js

@@ -52,6 +52,11 @@ describe('removeJSComments', () => {
     );
   });
 
+  it('leaves malformed JS unchanged', () => {
+    const actual = '/ unclosed regex';
+    expect(removeJSComments(actual)).toBe(actual);
+  });
+
   it('does not remove a url found in JS code', () => {
     expect(removeJSComments(jsCodeWithUrl)).toBe(jsCodeWithUrlUnchanged);
   });

curriculum/test/test-challenges.js

@@ -519,7 +519,7 @@ async function createTestRunner(
   buildChallenge,
   solutionFromNext
 ) {
-  const { required = [], template } = challenge;
+  const { required = [], template, removeComments } = challenge;
   // we should avoid modifying challenge, as it gets reused:
   const files = cloneDeep(challenge.files);
 
@@ -528,16 +528,11 @@ async function createTestRunner(
     files[key].editableContents = solution[key].editableContents;
   });
 
-  const { build, sources, loadEnzyme } = await buildChallenge(
-    {
-      files,
-      required,
-      template
-    },
-    {
-      removeComments: challenge.removeComments
-    }
-  );
+  const { build, sources, loadEnzyme } = await buildChallenge({
+    files,
+    required,
+    template
+  });
 
   const code = {
     contents: sources.index,
@@ -546,7 +541,7 @@ async function createTestRunner(
 
   const evaluator = await (buildChallenge === buildDOMChallenge
     ? getContextEvaluator(build, sources, code, loadEnzyme)
-    : getWorkerEvaluator(build, sources, code));
+    : getWorkerEvaluator(build, sources, code, removeComments));
 
   return async ({ text, testString }) => {
     try {
@@ -587,12 +582,14 @@ async function getContextEvaluator(build, sources, code, loadEnzyme) {
   };
 }
 
-async function getWorkerEvaluator(build, sources, code) {
+async function getWorkerEvaluator(build, sources, code, removeComments) {
   const testWorker = createWorker(testEvaluator, { terminateWorker: true });
   return {
     evaluate: async (testString, timeout) =>
-      await testWorker.execute({ testString, build, code, sources }, timeout)
-        .done
+      await testWorker.execute(
+        { testString, build, code, sources, removeComments },
+        timeout
+      ).done
   };
 }