diff --git a/client/src/client/workers/test-evaluator.js b/client/src/client/workers/test-evaluator.js index c6415642cd..13adf0cb60 100644 --- a/client/src/client/workers/test-evaluator.js +++ b/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); diff --git a/client/src/templates/Challenges/classic/Show.js b/client/src/templates/Challenges/classic/Show.js index 45315eeee4..de9a7dd066 100644 --- a/client/src/templates/Challenges/classic/Show.js +++ b/client/src/templates/Challenges/classic/Show.js @@ -172,7 +172,7 @@ class ShowClassic extends Component { updateChallengeMeta({ ...challengeMeta, title, - removeComments, + removeComments: removeComments !== false, challengeType, helpCategory }); diff --git a/client/src/templates/Challenges/redux/execute-challenge-saga.js b/client/src/templates/Challenges/redux/execute-challenge-saga.js index 436ccfa6ed..689ad270c8 100644 --- a/client/src/templates/Challenges/redux/execute-challenge-saga.js +++ b/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); } diff --git a/client/src/templates/Challenges/utils/build.js b/client/src/templates/Challenges/utils/build.js index 399ee5c0f4..9cc8632737 100644 --- a/client/src/templates/Challenges/utils/build.js +++ b/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 }) { diff --git a/client/src/utils/curriculum-helpers.test.js b/client/src/utils/curriculum-helpers.test.js index 7861742758..b6d3bada45 100644 --- a/client/src/utils/curriculum-helpers.test.js +++ b/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); }); diff --git a/curriculum/test/test-challenges.js b/curriculum/test/test-challenges.js index b4840dedd1..570ac07c9d 100644 --- a/curriculum/test/test-challenges.js +++ b/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 }; }