From 0d3158d4f4acdc8392194a8e63fa5d5b4ae0b1cc Mon Sep 17 00:00:00 2001 From: Oliver Eyton-Williams Date: Sat, 20 Mar 2021 07:29:13 +0100 Subject: [PATCH] fix(tests): add validation of comment translations (#41537) --- curriculum/package-lock.json | 16 +- curriculum/package.json | 1 - curriculum/schema/challengeSchema.js | 2 + curriculum/test/test-challenges.js | 35 ++- .../test/utils/extract-css-comments.test.js | 47 ++++ .../test/utils/extract-script-js-comments.js | 12 + .../utils/extract-script-js-comments.test.js | 77 +++++ .../test/utils/plugins/get-css-comments.js | 30 +- .../utils/plugins/get-script-js-comments.js | 29 ++ .../__fixtures__/challenge-objects.js | 236 ---------------- .../translation-parser/index.js | 123 +++----- .../translation-parser/index.test.js | 264 ++++++------------ 12 files changed, 345 insertions(+), 527 deletions(-) create mode 100644 curriculum/test/utils/extract-script-js-comments.js create mode 100644 curriculum/test/utils/extract-script-js-comments.test.js create mode 100644 curriculum/test/utils/plugins/get-script-js-comments.js diff --git a/curriculum/package-lock.json b/curriculum/package-lock.json index e682f3fe60..cf48196651 100644 --- a/curriculum/package-lock.json +++ b/curriculum/package-lock.json @@ -38,10 +38,12 @@ "puppeteer": "^8.0.0", "readdirp": "^3.5.0", "rehype": "^11.0.0", - "rework-visit": "^1.0.0", "string-similarity": "^4.0.2", "unist-util-visit": "^2.0.3", "vfile": "^4.2.0" + }, + "engines": { + "node": ">= 14.0.0" } }, "node_modules/@babel/code-frame": { @@ -8090,12 +8092,6 @@ "node": ">=0.12" } }, - "node_modules/rework-visit": { - "version": "1.0.0", - "resolved": "https://registry.npmjs.org/rework-visit/-/rework-visit-1.0.0.tgz", - "integrity": "sha1-mUWygD8hni96ygCtuLyfZA+ELJo=", - "dev": true - }, "node_modules/rimraf": { "version": "3.0.2", "resolved": "https://registry.npmjs.org/rimraf/-/rimraf-3.0.2.tgz", @@ -16669,12 +16665,6 @@ "integrity": "sha512-TTlYpa+OL+vMMNG24xSlQGEJ3B/RzEfUlLct7b5G/ytav+wPrplCpVMFuwzXbkecJrb6IYo1iFb0S9v37754mg==", "dev": true }, - "rework-visit": { - "version": "1.0.0", - "resolved": "https://registry.npmjs.org/rework-visit/-/rework-visit-1.0.0.tgz", - "integrity": "sha1-mUWygD8hni96ygCtuLyfZA+ELJo=", - "dev": true - }, "rimraf": { "version": "3.0.2", "resolved": "https://registry.npmjs.org/rimraf/-/rimraf-3.0.2.tgz", diff --git a/curriculum/package.json b/curriculum/package.json index f3c4b2e5e1..6c6f81a12e 100644 --- a/curriculum/package.json +++ b/curriculum/package.json @@ -55,7 +55,6 @@ "puppeteer": "^8.0.0", "readdirp": "^3.5.0", "rehype": "^11.0.0", - "rework-visit": "^1.0.0", "string-similarity": "^4.0.2", "unist-util-visit": "^2.0.3", "vfile": "^4.2.0" diff --git a/curriculum/schema/challengeSchema.js b/curriculum/schema/challengeSchema.js index e46c87ae2f..72e1ff8799 100644 --- a/curriculum/schema/challengeSchema.js +++ b/curriculum/schema/challengeSchema.js @@ -27,6 +27,8 @@ const schema = Joi.object() challengeOrder: Joi.number(), challengeType: Joi.number().min(0).max(11).required(), checksum: Joi.number(), + // __commentCounts is only used to test the comment replacement + __commentCounts: Joi.object(), // TODO: require this only for normal challenges, not certs dashedName: Joi.string().regex(slugRE), description: Joi.when('challengeType', { diff --git a/curriculum/test/test-challenges.js b/curriculum/test/test-challenges.js index c1762e1c17..c022066f35 100644 --- a/curriculum/test/test-challenges.js +++ b/curriculum/test/test-challenges.js @@ -24,7 +24,7 @@ const { const { assert, AssertionError } = require('chai'); const Mocha = require('mocha'); -const { flatten, isEmpty, cloneDeep } = require('lodash'); +const { flatten, isEmpty, cloneDeep, isEqual } = require('lodash'); const { getLines } = require('../../utils/get-lines'); const jsdom = require('jsdom'); @@ -62,12 +62,14 @@ const TRANSLATABLE_COMMENTS = getTranslatableComments( // the config files are created during the build, but not before linting // eslint-disable-next-line import/no-unresolved const testEvaluator = require('../../config/client/test-evaluator').filename; +const { inspect } = require('util'); const commentExtractors = { html: require('./utils/extract-html-comments'), js: require('./utils/extract-js-comments'), jsx: require('./utils/extract-jsx-comments'), - css: require('./utils/extract-css-comments') + css: require('./utils/extract-css-comments'), + scriptJs: require('./utils/extract-script-js-comments') }; // rethrow unhandled rejections to make sure the tests exit with -1 @@ -329,7 +331,7 @@ function populateTestsForLang({ lang, challenges, meta }) { // We get all the actual comments using the appropriate parsers if (file.ext === 'html') { - const commentTypes = ['css', 'html']; + const commentTypes = ['css', 'html', 'scriptJs']; for (let type of commentTypes) { const newComments = commentExtractors[type](file.contents); for (const [key, value] of Object.entries(newComments)) { @@ -342,17 +344,22 @@ function populateTestsForLang({ lang, challenges, meta }) { comments = commentExtractors[file.ext](file.contents); } - // Then we compare the number of times a given comment appears - // (count) with the number of times the text within it appears - // (commentTextCount) - for (const [comment, count] of Object.entries(comments)) { - const commentTextCount = - file.contents.split(comment).length - 1; - if (commentTextCount !== count) - throw Error( - `Translated comment text, ${comment}, should only appear inside comments` - ); - } + // Then we compare the number of times each comment appears in the + // translated text (commentMap) with the number of replacements + // made during translation (challenge.__commentCounts). If they + // differ, the translation must have gone wrong + + const commentMap = new Map(Object.entries(comments)); + + if (isEmpty(challenge.__commentCounts) && isEmpty(commentMap)) + return; + + if (!isEqual(commentMap, challenge.__commentCounts)) + throw Error(`Mismatch in ${challenge.title}. Replaced comments: +${inspect(challenge.__commentCounts)} +Comments in translated text: +${inspect(commentMap)} +`); }); }); diff --git a/curriculum/test/utils/extract-css-comments.test.js b/curriculum/test/utils/extract-css-comments.test.js index 70d71e8081..52088f2137 100644 --- a/curriculum/test/utils/extract-css-comments.test.js +++ b/curriculum/test/utils/extract-css-comments.test.js @@ -19,6 +19,39 @@ Some text `; +const outsideDeclarations = ` + +`; + +const mediaQuery = ` + +`; + // NOTE: this is a bit finicky. If the css is, say, missing a semi-colon, // nearby comments may be missed. describe('extractCSSComments', () => { @@ -29,4 +62,18 @@ describe('extractCSSComments', () => { }; expect(extractCSSComments(someHTMLWithCSS)).toEqual(commentCounts); }); + it('should catch comments outside of declarations', () => { + const commentCounts = { + 'comment 1': 2, + 'comment 2': 1 + }; + expect(extractCSSComments(outsideDeclarations)).toEqual(commentCounts); + }); + it('should catch comments inside of media queries', () => { + const commentCounts = { + 'comment 1': 1, + 'comment 2': 2 + }; + expect(extractCSSComments(mediaQuery)).toEqual(commentCounts); + }); }); diff --git a/curriculum/test/utils/extract-script-js-comments.js b/curriculum/test/utils/extract-script-js-comments.js new file mode 100644 index 0000000000..2767e2291b --- /dev/null +++ b/curriculum/test/utils/extract-script-js-comments.js @@ -0,0 +1,12 @@ +var rehype = require('rehype'); +var vfile = require('vfile'); + +var getComments = require('./plugins/get-script-js-comments'); + +const processor = rehype().use(getComments); + +function extractComments(html) { + return processor.processSync(vfile(html)).data; +} + +module.exports = extractComments; diff --git a/curriculum/test/utils/extract-script-js-comments.test.js b/curriculum/test/utils/extract-script-js-comments.test.js new file mode 100644 index 0000000000..f6a759b037 --- /dev/null +++ b/curriculum/test/utils/extract-script-js-comments.test.js @@ -0,0 +1,77 @@ +/* global expect */ +const extractScriptJSComments = require('./extract-script-js-comments'); + +const inlineComments = ` +Some text + + +`; + +const multilineComments = ` +Some text + + +`; + +const outsideScript = ` +Some text + + +// comment 2 + +`; + +describe('extractScriptJSComments', () => { + it('should catch inline comments', () => { + const commentCounts = { + 'comment 1': 2, + 'comment 2': 1 + }; + expect(extractScriptJSComments(inlineComments)).toEqual(commentCounts); + }); + it('should catch multiline comments', () => { + const commentCounts = { + 'comment 1': 2, + 'comment 2': 1 + }; + expect(extractScriptJSComments(multilineComments)).toEqual(commentCounts); + }); + it('should ignore comments outside script tags', () => { + const commentCounts = { + 'comment 1': 2, + 'comment 2': 1 + }; + expect(extractScriptJSComments(outsideScript)).toEqual(commentCounts); + }); +}); diff --git a/curriculum/test/utils/plugins/get-css-comments.js b/curriculum/test/utils/plugins/get-css-comments.js index ec93142069..355faf9a3c 100644 --- a/curriculum/test/utils/plugins/get-css-comments.js +++ b/curriculum/test/utils/plugins/get-css-comments.js @@ -1,9 +1,28 @@ const { isEmpty } = require('lodash'); const visit = require('unist-util-visit'); var css = require('css'); -var visitCss = require('rework-visit'); const { commentToData } = require('../comment-to-data'); +function visitComments(node, cb) { + node.rules.forEach(rule => { + if (rule.type === 'rule') { + visitDeclarations(rule.declarations, cb); + } else if (rule.type === 'comment') { + cb(rule.comment); + } else if (rule.type === 'media') { + visitComments(rule, cb); + } + }); +} + +function visitDeclarations(declarations, cb) { + declarations.forEach(dec => { + if (dec.type === 'comment') { + cb(dec.comment); + } + }); +} + function plugin() { return transformer; @@ -16,12 +35,9 @@ function plugin() { } function cssVisitor(node) { const ast = css.parse(node.value); - visitCss(ast.stylesheet, dec => { - let comments = dec - .filter(({ type }) => type === 'comment') - .map(({ comment }) => comment.trim()); - comments.forEach(comment => commentToData(file, comment)); - }); + visitComments(ast.stylesheet, comment => + commentToData(file, comment.trim()) + ); } } } diff --git a/curriculum/test/utils/plugins/get-script-js-comments.js b/curriculum/test/utils/plugins/get-script-js-comments.js new file mode 100644 index 0000000000..d50edaf87e --- /dev/null +++ b/curriculum/test/utils/plugins/get-script-js-comments.js @@ -0,0 +1,29 @@ +const { isEmpty } = require('lodash'); +const visit = require('unist-util-visit'); +const acorn = require('acorn'); +const { commentToData } = require('../comment-to-data'); + +const parser = acorn.Parser; + +function plugin() { + return transformer; + + function transformer(tree, file) { + if (isEmpty(file.data)) file.data = {}; + visit(tree, { type: 'element', tagName: 'script' }, scriptVisitor); + + function scriptVisitor(node) { + visit(node, 'text', jsVisitor); + } + function jsVisitor(node) { + let comments = []; + parser.parse(node.value, { onComment: comments, ecmaVersion: 2020 }); + + comments + .map(({ value }) => value.trim()) + .forEach(comment => commentToData(file, comment)); + } + } +} + +module.exports = plugin; diff --git a/tools/challenge-parser/translation-parser/__fixtures__/challenge-objects.js b/tools/challenge-parser/translation-parser/__fixtures__/challenge-objects.js index db06f99c43..a17e7b4f49 100644 --- a/tools/challenge-parser/translation-parser/__fixtures__/challenge-objects.js +++ b/tools/challenge-parser/translation-parser/__fixtures__/challenge-objects.js @@ -1,92 +1,3 @@ -const ENGLISH_CERTIFICATE = { - id: '561add10cb82ac38a17513bc', - title: 'Responsive Web Design Certificate', - challengeType: 7, - isPrivate: true, - tests: [ - { id: 'bd7158d8c442eddfaeb5bd18', title: 'Build a Tribute Page' }, - { id: '587d78af367417b2b2512b03', title: 'Build a Survey Form' }, - { - id: '587d78af367417b2b2512b04', - title: 'Build a Product Landing Page' - }, - { - id: '587d78b0367417b2b2512b05', - title: 'Build a Technical Documentation Page' - }, - { - id: 'bd7158d8c242eddfaeb5bd13', - title: 'Build a Personal Portfolio Webpage' - } - ], - solutions: ['// solution required\n'], - description: '', - instructions: '', - files: [] -}; - -const ENGLISH_CHALLENGE = { - id: 'id', - title: 'Title', - challengeType: 0, - videoUrl: 'https://scrimba.com/', - forumTopicId: 12345, - tests: [ - { - text: 'Test text', - testString: 'assertions' - }, - { - text: 'Test text2', - testString: 'assertions2' - } - ], - solutions: ['solution html string'], - description: 'description html string', - instructions: 'instructions html string', - files: [ - { - key: 'indexhtml', - ext: 'html', - name: 'index', - contents: 'seed html string', - head: 'head string', - tail: 'tail string' - } - ] -}; - -const ENGLISH_CHALLENGE_TWO_SOLUTIONS = { - id: 'id', - title: 'Title', - challengeType: 0, - videoUrl: 'https://scrimba.com/', - forumTopicId: 12345, - tests: [ - { - text: 'Test text', - testString: 'assertions' - }, - { - text: 'Test text2', - testString: 'assertions2' - } - ], - solutions: ['solution html string', 'second solution html string'], - description: 'description html string', - instructions: 'instructions html string', - files: [ - { - key: 'indexhtml', - ext: 'html', - name: 'index', - contents: 'seed html string', - head: '', - tail: '' - } - ] -}; - const ENGLISH_CHALLENGE_NO_FILES = { id: 'id', title: 'Title', @@ -109,151 +20,4 @@ const ENGLISH_CHALLENGE_NO_FILES = { files: [] }; -const ENGLISH_VIDEO_CHALLENGE = { - id: 'id', - title: 'Title', - challengeType: 0, - videoId: 'abc123', - forumTopicId: 12345, - question: 'english question', - description: 'description html string', - instructions: 'instructions html string' -}; - -const TRANSLATED_CERTIFICATE = { - id: '561add10cb82ac38a17513bc', - title: '响应式网页设计证书', - challengeType: 7, - isPrivate: true, - videoUrl: '', - tests: [ - { id: 'bd7158d8c442eddfaeb5bd18', title: 'Build a Tribute Page' }, - { id: '587d78af367417b2b2512b03', title: 'Build a Survey Form' }, - { - id: '587d78af367417b2b2512b04', - title: 'Build a Product Landing Page' - }, - { - id: '587d78b0367417b2b2512b05', - title: 'Build a Technical Documentation Page' - }, - { - id: 'bd7158d8c242eddfaeb5bd13', - title: 'Build a Personal Portfolio Webpage' - } - ], - solutions: ['// solution required\n'], - description: '', - instructions: '', - files: [] -}; - -const TRANSLATED_CHALLENGE = { - id: 'id', - title: 'Translated title', - challengeType: 0, - videoUrl: 'https://scrimba.com/', - forumTopicId: 9876, - tests: [ - { - text: 'Translated test text', - testString: 'Translated assertions, should be ignored' - }, - { - text: 'Translated test text2', - testString: 'Translated assertions, should be ignored2' - } - ], - solutions: ['Translated solution html string, should be ignored'], - description: 'Translated description html string', - instructions: 'Translated instructions html string', - files: [ - { - key: 'indexhtml', - ext: 'html', - name: 'index', - contents: 'Translated seed html string, should be ignored', - head: 'Translated head string, should be ignored', - tail: 'Translated tail string, should be ignored' - } - ] -}; - -const TRANSLATED_CHALLENGE_NO_TITLE = { - id: 'id', - challengeType: 0, - videoUrl: 'https://scrimba.com/', - forumTopicId: 9876, - tests: [ - { - text: 'Translated test text', - testString: 'Translated assertions, should be ignored' - }, - { - text: 'Translated test text2', - testString: 'Translated assertions, should be ignored2' - } - ], - solutions: ['Translated solution html string, should be ignored'], - description: 'Translated description html string', - instructions: 'Translated instructions html string', - files: [ - { - key: 'indexhtml', - ext: 'html', - name: 'index', - contents: 'Translated seed html string, should be ignored', - head: 'Translated head string, should be ignored', - tail: 'Translated tail string, should be ignored' - } - ] -}; - -const TRANSLATED_VIDEO_CHALLENGE = { - id: 'id', - title: 'Title', - challengeType: 0, - videoId: 'abc123', - forumTopicId: 12345, - question: 'translated question', - description: 'translated description html string', - instructions: 'translated instructions html string' -}; - -const WRONG_NUM_TESTS_CHALLENGE = { - id: 'id', - title: 'Translated title', - challengeType: 0, - videoUrl: 'https://scrimba.com/', - forumTopicId: 12345, - tests: [ - { - text: 'Translated test text', - testString: 'Translated assertions, should be ignored' - } - ], - solutions: ['Translated solution html string, should be ignored'], - description: 'Translated description html string', - instructions: 'Translated instructions html string', - files: [ - { - key: 'indexhtml', - ext: 'html', - name: 'index', - contents: 'Translated seed html string, should be ignored', - head: '', - tail: '' - } - ] -}; - -exports.ENGLISH_CERTIFICATE = ENGLISH_CERTIFICATE; -exports.ENGLISH_CHALLENGE = ENGLISH_CHALLENGE; -exports.ENGLISH_CHALLENGE_TWO_SOLUTIONS = ENGLISH_CHALLENGE_TWO_SOLUTIONS; exports.ENGLISH_CHALLENGE_NO_FILES = ENGLISH_CHALLENGE_NO_FILES; -exports.ENGLISH_VIDEO_CHALLENGE = ENGLISH_VIDEO_CHALLENGE; -exports.TRANSLATED_CERTIFICATE = TRANSLATED_CERTIFICATE; -exports.TRANSLATED_CHALLENGE = TRANSLATED_CHALLENGE; -exports.TRANSLATED_CHALLENGE_NO_TITLE = TRANSLATED_CHALLENGE_NO_TITLE; -exports.TRANSLATED_VIDEO_CHALLENGE = TRANSLATED_VIDEO_CHALLENGE; -exports.WRONG_NUM_TESTS_CHALLENGE = WRONG_NUM_TESTS_CHALLENGE; diff --git a/tools/challenge-parser/translation-parser/index.js b/tools/challenge-parser/translation-parser/index.js index a3dc49a818..4b7c3ee459 100644 --- a/tools/challenge-parser/translation-parser/index.js +++ b/tools/challenge-parser/translation-parser/index.js @@ -1,16 +1,17 @@ -const { isEmpty, cloneDeep } = require('lodash'); +const { cloneDeep } = require('lodash'); exports.translateComments = (text, lang, dict, codeLang) => { const knownComments = Object.keys(dict); const config = { knownComments, dict, lang }; + const input = { text, commentCounts: new Map() }; switch (codeLang) { case 'js': case 'jsx': - return transMultiline(transInline(text, config), config); + return transMultiline(transInline(input, config), config); case 'html': - return transScript(transHTML(transCSS(text, config), config), config); + return transScript(transHTML(transCSS(input, config), config), config); default: - return text; + return input; } }; @@ -21,125 +22,91 @@ exports.translateCommentsInChallenge = (challenge, lang, dict) => { } else { Object.keys(challClone.files).forEach(key => { if (challClone.files[key].contents) { - challClone.files[key].contents = this.translateComments( + let { text, commentCounts } = this.translateComments( challenge.files[key].contents, lang, dict, challClone.files[key].ext ); + challClone.__commentCounts = commentCounts; + challClone.files[key].contents = text; } }); } return challClone; }; -exports.mergeChallenges = (engChal, transChal) => { - const hasTests = - (engChal.tests && transChal.tests) || - (engChal.question && transChal.question); - const challenge = { - ...engChal, - description: transChal.description, - instructions: transChal.instructions, - originalTitle: engChal.title, - // TODO: throw in production? - title: isEmpty(transChal.title) ? engChal.title : transChal.title, - forumTopicId: transChal.forumTopicId - }; - if (!hasTests) - throw Error( - `Both challenges must have tests or questions. - title: ${engChal.title} - translated title: ${transChal.title}` - ); - // TODO: this should break the build when we go to production, but - // not for testing. - if (transChal.tests && transChal.tests.length !== engChal.tests.length) { - console.error( - `Challenges in both languages must have the same number of tests. - title: ${engChal.title} - translated title: ${transChal.title}` - ); - return challenge; - } - - // throw Error( - // `Challenges in both languages must have the same number of tests. - // title: ${engChal.title} - // translated title: ${transChal.title}` - // ); - - if (transChal.tests) { - const translatedTests = - engChal.challengeType === 7 - ? transChal.tests.map(({ title }, i) => ({ - title, - id: engChal.tests[i].id - })) - : transChal.tests.map(({ text }, i) => ({ - text, - testString: engChal.tests[i].testString - })); - challenge.tests = translatedTests; - } else { - challenge.question = transChal.question; - } - - // certificates do not have forumTopicIds - if (challenge.challengeType === 7) delete challenge.forumTopicId; - return challenge; -}; - // bare urls could be interpreted as comments, so we have to lookbehind for // http:// or https:// -function transInline(text, config) { - return translateGeneric(text, config, '((?.*?<\/style>/gms; const matches = text.matchAll(regex); for (const [match] of matches) { - text = text.replace(match, transMultiline(match, config)); + let { text: styleText } = transMultiline( + { text: match, commentCounts }, + config + ); + text = text.replace(match, styleText); } - return text; + return { text, commentCounts }; } -function transScript(text, config) { +function transScript({ text, commentCounts }, config) { const regex = /`; expect( - translateComments(seed, 'chinese', SIMPLE_TRANSLATION, 'html') + translateComments(seed, 'chinese', SIMPLE_TRANSLATION, 'html').text ).toBe(transSeed); }); @@ -351,23 +259,23 @@ describe('translation parser', () => { // (Chinese) Add your code below this line (Chinese) `; expect( - translateComments(seed, 'chinese', SIMPLE_TRANSLATION, 'html') + translateComments(seed, 'chinese', SIMPLE_TRANSLATION, 'html').text ).toBe(transSeed); }); it('ignores html comments inside JavaScript', () => { const seed = `
change code below this line `; - expect(translateComments(seed, 'chinese', SIMPLE_TRANSLATION, 'js')).toBe( - seed - ); + expect( + translateComments(seed, 'chinese', SIMPLE_TRANSLATION, 'js').text + ).toBe(seed); }); it('ignores html comments inside jsx', () => { const seed = `
change code below this line `; expect( - translateComments(seed, 'chinese', SIMPLE_TRANSLATION, 'jsx') + translateComments(seed, 'chinese', SIMPLE_TRANSLATION, 'jsx').text ).toBe(seed); }); @@ -407,7 +315,7 @@ describe('translation parser', () => { const seed = `{ /* Add your code below this line */ }`; const transSeed = `{ /* (Chinese) Add your code below this line (Chinese) */ }`; expect( - translateComments(seed, 'chinese', SIMPLE_TRANSLATION, 'jsx') + translateComments(seed, 'chinese', SIMPLE_TRANSLATION, 'jsx').text ).toBe(transSeed); });