From 68d8aa3021c3fad6f197ed5a7d02d6af06932124 Mon Sep 17 00:00:00 2001 From: Ze Yu Date: Sat, 28 Mar 2020 21:35:30 +0800 Subject: [PATCH] Expand test extensions and fix whitespace checks The diffing utilities used for functional tests only check for differences in html files and the siteData json file. Additionally, the diffs are whitespace and newline insensitive, which can cause subtle, unintended changes to go unnoticed. Let's expand the range of files to diff, using the isTextOrBinary package as a first guard against diffing binary files. Let's add additional constants that guard against binary files not recognised by the package, or misrecognised as such. Let's also change the diffing function used to be whitespace and newline sensitive. Since whitespaces and newlines now appear in the diffs, let's also improve the diff printing utilities to account for it. --- package-lock.json | 63 +++++++++++--- package.json | 1 + test/functional/testUtil/diffChars.js | 23 +++++ test/functional/testUtil/diffHtml.js | 25 ------ test/functional/testUtil/diffPrinter.js | 111 ++++++++++++++---------- test/functional/testUtil/test.js | 48 ++++++---- 6 files changed, 171 insertions(+), 100 deletions(-) create mode 100644 test/functional/testUtil/diffChars.js delete mode 100644 test/functional/testUtil/diffHtml.js diff --git a/package-lock.json b/package-lock.json index 601a5a64fd..9170a4d38d 100644 --- a/package-lock.json +++ b/package-lock.json @@ -1312,6 +1312,12 @@ "resolved": "https://registry.npmjs.org/binary-extensions/-/binary-extensions-2.0.0.tgz", "integrity": "sha512-Phlt0plgpIIBOGTT/ehfFnbNlfsDEiqmzE2KRXoX1bLIlir4X/MR+zSyBEkL05ffWgnRSf/DXv+WrUAVr93/ow==" }, + "binaryextensions": { + "version": "2.2.0", + "resolved": "https://registry.npmjs.org/binaryextensions/-/binaryextensions-2.2.0.tgz", + "integrity": "sha512-bHhs98rj/7i/RZpCSJ3uk55pLXOItjIrh2sRQZSM6OoktScX+LxJzvlU+FELp9j3TdcddTmmYArLSGptCTwjuw==", + "dev": true + }, "bluebird": { "version": "3.7.2", "resolved": "https://registry.npmjs.org/bluebird/-/bluebird-3.7.2.tgz", @@ -3161,7 +3167,8 @@ }, "ansi-regex": { "version": "2.1.1", - "bundled": true + "bundled": true, + "optional": true }, "aproba": { "version": "1.2.0", @@ -3179,11 +3186,13 @@ }, "balanced-match": { "version": "1.0.0", - "bundled": true + "bundled": true, + "optional": true }, "brace-expansion": { "version": "1.1.11", "bundled": true, + "optional": true, "requires": { "balanced-match": "^1.0.0", "concat-map": "0.0.1" @@ -3196,15 +3205,18 @@ }, "code-point-at": { "version": "1.1.0", - "bundled": true + "bundled": true, + "optional": true }, "concat-map": { "version": "0.0.1", - "bundled": true + "bundled": true, + "optional": true }, "console-control-strings": { "version": "1.1.0", - "bundled": true + "bundled": true, + "optional": true }, "core-util-is": { "version": "1.0.2", @@ -3307,7 +3319,8 @@ }, "inherits": { "version": "2.0.3", - "bundled": true + "bundled": true, + "optional": true }, "ini": { "version": "1.3.5", @@ -3317,6 +3330,7 @@ "is-fullwidth-code-point": { "version": "1.0.0", "bundled": true, + "optional": true, "requires": { "number-is-nan": "^1.0.0" } @@ -3329,17 +3343,20 @@ "minimatch": { "version": "3.0.4", "bundled": true, + "optional": true, "requires": { "brace-expansion": "^1.1.7" } }, "minimist": { "version": "0.0.8", - "bundled": true + "bundled": true, + "optional": true }, "minipass": { "version": "2.3.5", "bundled": true, + "optional": true, "requires": { "safe-buffer": "^5.1.2", "yallist": "^3.0.0" @@ -3356,6 +3373,7 @@ "mkdirp": { "version": "0.5.1", "bundled": true, + "optional": true, "requires": { "minimist": "0.0.8" } @@ -3428,7 +3446,8 @@ }, "number-is-nan": { "version": "1.0.1", - "bundled": true + "bundled": true, + "optional": true }, "object-assign": { "version": "4.1.1", @@ -3438,6 +3457,7 @@ "once": { "version": "1.4.0", "bundled": true, + "optional": true, "requires": { "wrappy": "1" } @@ -3513,7 +3533,8 @@ }, "safe-buffer": { "version": "5.1.2", - "bundled": true + "bundled": true, + "optional": true }, "safer-buffer": { "version": "2.1.2", @@ -3543,6 +3564,7 @@ "string-width": { "version": "1.0.2", "bundled": true, + "optional": true, "requires": { "code-point-at": "^1.0.0", "is-fullwidth-code-point": "^1.0.0", @@ -3560,6 +3582,7 @@ "strip-ansi": { "version": "3.0.1", "bundled": true, + "optional": true, "requires": { "ansi-regex": "^2.0.0" } @@ -3598,11 +3621,13 @@ }, "wrappy": { "version": "1.0.2", - "bundled": true + "bundled": true, + "optional": true }, "yallist": { "version": "3.0.3", - "bundled": true + "bundled": true, + "optional": true } } }, @@ -4559,6 +4584,16 @@ "istanbul-lib-report": "^3.0.0" } }, + "istextorbinary": { + "version": "3.3.0", + "resolved": "https://registry.npmjs.org/istextorbinary/-/istextorbinary-3.3.0.tgz", + "integrity": "sha512-Tvq1W6NAcZeJ8op+Hq7tdZ434rqnMx4CCZ7H0ff83uEloDvVbqAwaMTZcafKGJT0VHkYzuXUiCY4hlXQg6WfoQ==", + "dev": true, + "requires": { + "binaryextensions": "^2.2.0", + "textextensions": "^3.2.0" + } + }, "jest": { "version": "25.1.0", "resolved": "https://registry.npmjs.org/jest/-/jest-25.1.0.tgz", @@ -8755,6 +8790,12 @@ "integrity": "sha1-f17oI66AUgfACvLfSoTsP8+lcLQ=", "dev": true }, + "textextensions": { + "version": "3.3.0", + "resolved": "https://registry.npmjs.org/textextensions/-/textextensions-3.3.0.tgz", + "integrity": "sha512-mk82dS8eRABNbeVJrEiN5/UMSCliINAuz8mkUwH4SwslkNP//gbEzlWNS5au0z5Dpx40SQxzqZevZkn+WYJ9Dw==", + "dev": true + }, "throat": { "version": "5.0.0", "resolved": "https://registry.npmjs.org/throat/-/throat-5.0.0.tgz", diff --git a/package.json b/package.json index afc81daf22..96c54a79e8 100644 --- a/package.json +++ b/package.json @@ -89,6 +89,7 @@ "eslint-config-airbnb-base": "^14.0.0", "eslint-plugin-import": "^2.18.2", "eslint-plugin-lodash": "^6.0.0", + "istextorbinary": "^3.3.0", "jest": "^25.1.0", "memfs": "^3.0.1", "stylelint": "^12.0.0", diff --git a/test/functional/testUtil/diffChars.js b/test/functional/testUtil/diffChars.js new file mode 100644 index 0000000000..52399363a9 --- /dev/null +++ b/test/functional/testUtil/diffChars.js @@ -0,0 +1,23 @@ +const jsdiff = require('diff'); +const DiffPrinter = require('./diffPrinter'); + +/** + * Checks for any diffs between expected.html and actual.html, + * then prints the differences. + * @param {string} expected + * @param {string} actual + * @param {string} filePathName + * @returns {boolean} if diff was found + */ +const diffCharsAndPrint = (expected, actual, filePathName) => { + const diffParts = jsdiff.diffChars(expected, actual); + const isDiff = part => part.added || part.removed; + const hasDiff = diffParts.some(isDiff); + if (hasDiff) { + DiffPrinter.printDiffFoundMessage(filePathName); + DiffPrinter.printDiff(diffParts); + } + return hasDiff; +}; + +module.exports = diffCharsAndPrint; diff --git a/test/functional/testUtil/diffHtml.js b/test/functional/testUtil/diffHtml.js deleted file mode 100644 index 901a45863d..0000000000 --- a/test/functional/testUtil/diffHtml.js +++ /dev/null @@ -1,25 +0,0 @@ -const jsdiff = require('diff'); -const DiffPrinter = require('./diffPrinter'); - -/** - * Checks for any diffs between expected.html and actual.html - * @param {string} expected - * @param {string} actual - * @param {string} filePathName - * @returns {bool} if diff was found - */ -const diffHtml = (expected, actual, filePathName) => { - const diffParts = jsdiff.diffWords(expected, actual); - const isDiff = part => part.added || part.removed; - const hasDiff = diffParts.some(isDiff); - if (hasDiff) { - DiffPrinter.printLine(); - DiffPrinter.printLine('-------------------------------------', 'grey'); - DiffPrinter.printLine(`Diff found in ${filePathName}`, 'grey'); - DiffPrinter.printLine(); - DiffPrinter.printDiff(diffParts); - } - return hasDiff; -}; - -module.exports = diffHtml; diff --git a/test/functional/testUtil/diffPrinter.js b/test/functional/testUtil/diffPrinter.js index e9c1b74ccb..fc9931096d 100644 --- a/test/functional/testUtil/diffPrinter.js +++ b/test/functional/testUtil/diffPrinter.js @@ -1,61 +1,73 @@ -const ANSI_RED = '\u001B[31m'; -const ANSI_GREEN = '\u001B[32m'; -const ANSI_GREY = '\u001B[90m'; -const ANSI_RESET = '\u001B[0m'; +const chalk = require('chalk'); + +const EMPTY_LINE = '|-------------------empty-line-------------------|'; +const CONSECUTIVE_NEWLINE_REGEX = new RegExp('\\n{2,}', 'g'); +const WHITESPACE_REGEX = new RegExp('\\s+', 'g'); class DiffPrinter { /** - * Prints line of text in colour provided - * @param {string} text text to print, default: no text - * @param {string} colour colour of text, default: no colour + * Replaces all newlines except the first with EMPTY_LINE. */ - static printLine(text = '', colour = 'none') { - let ansiEscCode = ''; - switch (colour) { - case 'red': - ansiEscCode = ANSI_RED; - break; - case 'green': - ansiEscCode = ANSI_GREEN; - break; - case 'grey': - ansiEscCode = ANSI_GREY; - break; - default: - ansiEscCode = ''; - break; + static prependNewLines(match) { + return `\n${match.replace('\n', '').split('\n').join(`${EMPTY_LINE}\n`)}`; + } + + static formatNewLines(value, prevVal, nextVal) { + let printValue; + printValue = value.replace(CONSECUTIVE_NEWLINE_REGEX, this.prependNewLines); + + /** + * Replace consecutive newlines between current value and adjacent values + */ + + const currentValStartsWithNewLine = printValue.startsWith('\n'); + const prevValEndsWithNewLine = prevVal && prevVal.endsWith('\n'); + if (currentValStartsWithNewLine && prevValEndsWithNewLine) { + printValue = EMPTY_LINE + printValue; } - process.stderr.write(`${ansiEscCode}${text}${ANSI_RESET}\n`); + + const currentValEndsWithNewLine = printValue.endsWith('\n'); + const nextValStartsWithNewLine = nextVal && nextVal.startsWith('\n'); + if (currentValEndsWithNewLine && nextValStartsWithNewLine) { + printValue += EMPTY_LINE; + } + + return printValue; } /** * Splits and combines change objects such that their value contains a single line * Also adds ANSI Escape Codes for diffs and unchanged lines - * @param {Array} parts array of change objects returned by jsdiff#diffWords + * @param {Array} diffObjects array of change objects returned by jsdiff#diffWords * @returns {Array} change objects where their value contains a single line */ - static generateLineParts(parts) { - let lineParts = [{ value: '' }]; - parts.forEach(({ value, added, removed }) => { - let lines = value.split(/\n/); - let asciEscCode = ANSI_GREY; - if (added) asciEscCode = ANSI_GREEN; - else if (removed) asciEscCode = ANSI_RED; - lines = lines.map(line => ({ - value: asciEscCode + line + ANSI_RESET, - diff: added || removed, - })); - - if (lines.length) { - const prevPart = lineParts.pop(); - lines[0] = { - value: prevPart.value + lines[0].value, - diff: lines[0].diff || prevPart.diff, - }; + static generateLineParts(diffObjects) { + const parts = []; + diffObjects.forEach(({ value, added, removed }, i) => { + let printValue = value; + if (added || removed) { + printValue = this.formatNewLines(printValue, + diffObjects[i - 1] && diffObjects[i - 1].value, + diffObjects[i + 1] && diffObjects[i - 1].value); + printValue = added + ? chalk.green(printValue.replace(WHITESPACE_REGEX, + match => chalk.bgGreenBright(match))) + : chalk.red(printValue.replace(WHITESPACE_REGEX, + match => chalk.bgRedBright(match))); + parts.push({ + value: printValue, + diff: true, + }); + } else { + // Split into lines only when it is untouched to avoid printing too much of the untouched areas + const lineParts = printValue.split('\n').map((line, index, lines) => ({ + value: (index === lines.length - 1) ? chalk.grey(line) : `${chalk.grey(line)}\n`, + diff: false, + })); + parts.push(...lineParts); } - lineParts = lineParts.concat(lines); }); - return lineParts; + return parts; } /** @@ -75,6 +87,11 @@ class DiffPrinter { }); } + static printDiffFoundMessage(filePath) { + const message = chalk.grey(`\n-------------------------------------\nDiff found in ${filePath}\n\n`); + process.stderr.write(message); + } + /** * Prints value in line objects that are set for printing * If there is a gap between lines, print ellipsis @@ -86,11 +103,9 @@ class DiffPrinter { const prevPart = lineParts[i - 1]; if (linePart.toPrint) { if (prevPart && !prevPart.toPrint) { - this.printLine(); - this.printLine('...', 'grey'); - this.printLine(); + process.stderr.write(chalk.grey('\n...\n')); } - this.printLine(linePart.value, 'none'); // already has ANSI Escape Code + process.stderr.write(linePart.value); } }); } diff --git a/test/functional/testUtil/test.js b/test/functional/testUtil/test.js index 3ed786081f..c99daa8f9a 100644 --- a/test/functional/testUtil/test.js +++ b/test/functional/testUtil/test.js @@ -1,12 +1,26 @@ const fs = require('fs'); const path = require('path'); const program = require('commander'); +const ignore = require('ignore'); const walkSync = require('walk-sync'); -const diffHtml = require('./diffHtml'); +const { isBinary } = require('istextorbinary'); +const diffChars = require('./diffChars'); const _ = {}; _.isEqual = require('lodash/isEqual'); +// Other files to ignore / files with binary extensions not recognised by istextorbinary package +const TEST_BLACKLIST = ignore().add([ + '*.log', + '*.woff', + '*.woff2', +]); + +// Files that possibly have null characters but are not binary files +const NULL_WHITELIST = ignore().add(['vue-strap.min.js']); + +const CRLF_REGEX = new RegExp('\\r\\n', 'g'); + function readFileSync(...paths) { return fs.readFileSync(path.resolve(...paths), 'utf8'); } @@ -27,6 +41,7 @@ program throw new Error('Unequal number of files'); } + /* eslint-disable no-continue */ for (let i = 0; i < expectedPaths.length; i += 1) { const expectedFilePath = expectedPaths[i]; const actualFilePath = actualPaths[i]; @@ -35,24 +50,25 @@ program throw new Error('Different files built'); } - const parsed = path.parse(actualFilePath); - if (parsed.ext === '.html') { - // compare html files - const expected = readFileSync(expectedDirectory, expectedFilePath); - const actual = readFileSync(actualDirectory, actualFilePath); - const hasDiff = diffHtml(expected, actual, expectedFilePath); - error = error || hasDiff; - } else if (parsed.base === 'siteData.json') { - // compare site data - const expected = readFileSync(expectedDirectory, expectedFilePath); - const actual = readFileSync(actualDirectory, actualFilePath); - if (!_.isEqual(JSON.parse(expected), JSON.parse(actual))) { - throw new Error('Site data does not match with the expected file.'); - } + if (isBinary(expectedFilePath) || TEST_BLACKLIST.ignores(expectedFilePath)) { + continue; } + + const expected = readFileSync(expectedDirectory, expectedFilePath).replace(CRLF_REGEX, '\n'); + const actual = readFileSync(actualDirectory, actualFilePath).replace(CRLF_REGEX, '\n'); + + if (!NULL_WHITELIST.ignores(expectedFilePath) && isBinary(null, expected)) { + // eslint-disable-next-line no-console + console.warn(`Unrecognised file extension ${expectedFilePath} contains null characters, skipping`); + continue; + } + + const hasDiff = diffChars(expected, actual, expectedFilePath); + error = error || hasDiff; } + /* eslint-enable no-continue */ - if (error) throw new Error('Diffs found in .html files'); + if (error) throw new Error('Diffs found in files'); }); program.parse(process.argv);