From 3166b7f57bf45dfe0fe75f7286c7c5fd708b2d2b Mon Sep 17 00:00:00 2001 From: Jos de Jong Date: Mon, 7 Mar 2022 11:50:42 +0100 Subject: [PATCH] Gwhitney doctesting (#2471) * test: Add unit tests for all of the examples in (jsdoc) comments Uses the existing extraction of examples from tools/docgenerator.js Hence, for now this is limited to documentation of functions, but hopefully it can be extended to classes, units (and physical constants), and constants as well in the future. Exposes numerous errors in the examples, some of which are bugs; these are for now put on a known error list to be worked on, so that this PR does not change a huge number of source files. Also adds a test to check that all symbols are documented (which similarly doesn't really pass at the moment, and is patched to a hopefully temporary warning). * refactor: Make doc.test.js into a node test The source code is not available in its layout as in the repository in the browser tests, so the new doc testing can only occur in the node tests * Add simplifyCore, symbolicEqual, map, and resolve to the list with functions with known issues in the jsdoc examples Co-authored-by: Glen Whitney --- test/node-tests/doc.test.js | 386 ++++++++++++++++++++++++++++++++++++ tools/docgenerator.js | 286 +++++++++++++------------- 2 files changed, 535 insertions(+), 137 deletions(-) create mode 100644 test/node-tests/doc.test.js diff --git a/test/node-tests/doc.test.js b/test/node-tests/doc.test.js new file mode 100644 index 000000000..2b2c244e8 --- /dev/null +++ b/test/node-tests/doc.test.js @@ -0,0 +1,386 @@ +const assert = require('assert') +const path = require('path') + +const approx = require('../../tools/approx.js') +const docgenerator = require('../../tools/docgenerator.js') +const math = require('../..') + +function extractExpectation (comment, optional = false) { + if (comment === '') return undefined + const returnsParts = comment.split('eturns').map(s => s.trim()) + if (returnsParts.length > 1) return extractValue(returnsParts[1]) + const outputsParts = comment.split('utputs') + if (outputsParts.length > 1) { + let output = outputsParts[1] + if (output[0] === ':') output = output.substring(1) + return extractValue(output.trim()) + } + // None of the usual flags; if we need a value, + // assume the whole comment is the desired value. Otherwise return undefined + if (optional) return undefined + return extractValue(comment) +} + +function extractValue (spec) { + // First check for a leading keyword: + const words = spec.split(' ') + // If the last word end in 'i' and the value is not labeled as complex, + // label it for the comment writer: + if (words[words.length - 1].substr(-1) === 'i' && words[0] !== 'Complex') { + words.unshift('Complex') + } + // Collapse 'Dense Matrix' into 'DenseMatrix' + if (words[0] === 'Dense' && words[1] === 'Matrix') { + words.shift() + words[0] = 'DenseMatrix' + } + const keywords = { + number: 'Number(_)', + BigNumber: 'math.bignumber(_)', + Fraction: 'math.fraction(_)', + Complex: "math.complex('_')", + Unit: "math.unit('_')", + Array: '_', + Matrix: 'math.matrix(_)', + DenseMatrix: "math.matrix(_, 'dense')", + string: '_', + Node: 'math.parse(_)', + throws: "'_'" + } + if (words[0] in keywords) { + const template = keywords[words[0]] + const spot = template.indexOf('_') + let filler = words.slice(1).join(' ') + if (words[0] === 'Complex') { // a bit of a hack here :( + filler = words.slice(1).join('') + } + spec = template.substring(0, spot) + filler + template.substr(spot + 1) + } + if (spec.substring(0, 7) === 'matrix(') { + spec = 'math.' + spec // More hackery :( + } + let value + try { + value = eval(spec) // eslint-disable-line no-eval + } catch (err) { + if (err instanceof SyntaxError || err instanceof ReferenceError) { + value = spec + } else { + throw err + } + } + if (words[0] === 'Unit') { // more hackishness here :( + value.fixPrefix = true + } + if (words[0] === 'Node') { // and even more :( + delete value.comment + } + return value +} + +const knownProblems = new Set([ + 'numeric', 'isZero', 'isPositive', 'isNumeric', 'isNegative', 'isNaN', + 'isInteger', 'hasNumericValue', 'clone', 'print', 'hex', 'format', 'to', 'sin', + 'cos', 'atan2', 'atan', 'asin', 'asec', 'acsc', 'acoth', 'acot', 'max', + 'setUnion', 'unequal', 'equal', 'deepEqual', 'compareNatural', 'randomInt', + 'random', 'pickRandom', 'kldivergence', 'xor', 'or', 'not', 'and', 'distance', + 'parser', 'compile', 're', 'im', 'rightLogShift', 'rightArithShift', + 'leftShift', 'bitNot', 'apply', 'subset', 'squeeze', 'rotationMatrix', + 'rotate', 'reshape', 'partitionSelect', 'matrixFromRows', 'matrixFromFunction', + 'matrixFromColumns', 'getMatrixDataType', 'forEach', 'eigs', 'diff', + 'ctranspose', 'concat', 'sqrtm', 'subtract', 'nthRoots', 'nthRoot', 'multiply', + 'mod', 'invmod', 'floor', 'fix', 'expm1', 'exp', 'dotPow', 'dotMultiply', + 'dotDivide', 'divide', 'ceil', 'cbrt', 'add', 'usolveAll', 'usolve', 'slu', + 'rationalize', 'qr', 'lusolve', 'lup', 'lsolveAll', 'lsolve', 'derivative', + 'simplifyCore', 'symbolicEqual', 'map', 'resolve' +]) + +function maybeCheckExpectation (name, expected, expectedFrom, got, gotFrom) { + if (knownProblems.has(name)) { + try { + checkExpectation(expected, got) + } catch (err) { + console.log( + `PLEASE RESOLVE: '${gotFrom}' was supposed to '${expectedFrom}'`) + console.log(' but', err.toString()) + } + } else { + checkExpectation(expected, got) + } +} + +function checkExpectation (want, got) { + if (Array.isArray(want) && !Array.isArray(got)) { + approx.deepEqual(got, math.matrix(want), 1e-9) + } else if (want instanceof math.Unit && got instanceof math.Unit) { + approx.deepEqual(got, want, 1e-9) + } else if (want instanceof math.Complex && got instanceof math.Complex) { + approx.deepEqual(got, want, 1e-9) + } else if (typeof want === 'number' && + typeof got === 'number' && + want !== got) { + approx.equal(got, want, 1e-9) + console.log(` Note: return value ${got} not exactly as expected: ${want}`) + } else { + assert.deepEqual(got, want) + } +} + +const OKundocumented = new Set([ + 'addScalar', 'divideScalar', 'multiplyScalar', 'equalScalar', + 'docs', 'FibonacciHeap', + 'IndexError', 'DimensionError', 'ArgumentsError' +]) + +const knownUndocumented = new Set([ + 'all', + 'isNumber', + 'isComplex', + 'isBigNumber', + 'isFraction', + 'isUnit', + 'isString', + 'isArray', + 'isMatrix', + 'isCollection', + 'isDenseMatrix', + 'isSparseMatrix', + 'isRange', + 'isIndex', + 'isBoolean', + 'isResultSet', + 'isHelp', + 'isFunction', + 'isDate', + 'isRegExp', + 'isObject', + 'isNull', + 'isUndefined', + 'isAccessorNode', + 'isArrayNode', + 'isAssignmentNode', + 'isBlockNode', + 'isConditionalNode', + 'isConstantNode', + 'isFunctionAssignmentNode', + 'isFunctionNode', + 'isIndexNode', + 'isNode', + 'isObjectNode', + 'isOperatorNode', + 'isParenthesisNode', + 'isRangeNode', + 'isSymbolNode', + 'isChain', + 'on', + 'off', + 'once', + 'emit', + 'config', + 'expression', + 'import', + 'create', + 'factory', + 'AccessorNode', + 'ArrayNode', + 'AssignmentNode', + 'atomicMass', + 'avogadro', + 'BigNumber', + 'bignumber', + 'BlockNode', + 'bohrMagneton', + 'bohrRadius', + 'boltzmann', + 'boolean', + 'chain', + 'Chain', + 'classicalElectronRadius', + 'complex', + 'Complex', + 'ConditionalNode', + 'conductanceQuantum', + 'ConstantNode', + 'coulomb', + 'createUnit', + 'DenseMatrix', + 'deuteronMass', + 'e', + 'efimovFactor', + 'electricConstant', + 'electronMass', + 'elementaryCharge', + 'false', + 'faraday', + 'fermiCoupling', + 'fineStructure', + 'firstRadiation', + 'fraction', + 'Fraction', + 'FunctionAssignmentNode', + 'FunctionNode', + 'gasConstant', + 'gravitationConstant', + 'gravity', + 'hartreeEnergy', + 'Help', + 'i', + 'ImmutableDenseMatrix', + 'index', + 'Index', + 'IndexNode', + 'Infinity', + 'inverseConductanceQuantum', + 'klitzing', + 'LN10', + 'LN2', + 'LOG10E', + 'LOG2E', + 'loschmidt', + 'magneticConstant', + 'magneticFluxQuantum', + 'matrix', + 'Matrix', + 'molarMass', + 'molarMassC12', + 'molarPlanckConstant', + 'molarVolume', + 'NaN', + 'neutronMass', + 'Node', + 'nuclearMagneton', + 'null', + 'number', + 'ObjectNode', + 'OperatorNode', + 'ParenthesisNode', + 'parse', + 'Parser', + 'phi', + 'pi', + 'planckCharge', + 'planckConstant', + 'planckLength', + 'planckMass', + 'planckTemperature', + 'planckTime', + 'protonMass', + 'quantumOfCirculation', + 'Range', + 'RangeNode', + 'reducedPlanckConstant', + 'RelationalNode', + 'replacer', + 'ResultSet', + 'reviver', + 'rydberg', + 'SQRT1_2', + 'SQRT2', + 'sackurTetrode', + 'secondRadiation', + 'Spa', + 'sparse', + 'SparseMatrix', + 'speedOfLight', + 'splitUnit', + 'stefanBoltzmann', + 'string', + 'SymbolNode', + 'tau', + 'thomsonCrossSection', + 'true', + 'typed', + 'Unit', + 'unit', + 'E', + 'PI', + 'vacuumImpedance', + 'version', + 'weakMixingAngle', + 'wienDisplacement' +]) + +const bigwarning = `WARNING: ${knownProblems.size} known errors converted ` + + 'to PLEASE RESOLVE warnings.' + + `\n WARNING: ${knownUndocumented.size} symbols in math are known to ` + + 'be undocumented; PLEASE EXTEND the documentation.' + +describe(bigwarning + '\n Testing examples from (jsdoc) comments', () => { + const allNames = Object.keys(math) + const srcPath = path.resolve(__dirname, '../../src') + '/' + const allDocs = docgenerator.collectDocs(allNames, srcPath) + it("should cover all names (but doesn't yet)", () => { + const documented = new Set(Object.keys(allDocs)) + const badUndocumented = allNames.filter(name => { + return !(documented.has(name) || + OKundocumented.has(name) || + knownUndocumented.has(name) || + name.substr(0, 1) === '_' || + name.substr(-12) === 'Dependencies' || + name.substr(0, 6) === 'create' + ) + }) + assert.deepEqual(badUndocumented, []) + }) + const byCategory = {} + for (const fun of Object.values(allDocs)) { + if (!(fun.category in byCategory)) { + byCategory[fun.category] = [] + } + byCategory[fun.category].push(fun.doc) + } + for (const category in byCategory) { + describe('category: ' + category, () => { + for (const doc of byCategory[category]) { + it('satisfies ' + doc.name, () => { + console.log(` Testing ${doc.name} ...`) // can remove once no known failures; for now it clarifies "PLEASE RESOLVE" + const lines = doc.examples + lines.push('//') // modifies doc but OK for test + let accumulation = '' + let expectation + let expectationFrom = '' + for (const line of lines) { + if (line.includes('//')) { + let parts = line.split('//') + if (parts[0] && !parts[0].trim()) { + // Indented comment, unusual in examples + // assume this is a comment within some code to evaluate + // i.e., ignore it + continue + } + // Comment specifying a future value or the return of prior code + parts = parts.map(s => s.trim()) + if (parts[0] !== '') { + if (accumulation) { accumulation += '\n' } + accumulation += parts[0] + } + if (accumulation !== '' && expectation === undefined) { + expectationFrom = parts[1] + expectation = extractExpectation(expectationFrom) + parts[1] = '' + } + if (accumulation) { + let value + try { + value = eval(accumulation) // eslint-disable-line no-eval + } catch (err) { + value = err.toString() + } + maybeCheckExpectation( + doc.name, expectation, expectationFrom, value, accumulation) + accumulation = '' + } + expectationFrom = parts[1] + expectation = extractExpectation(expectationFrom, 'requireSignal') + } else { + if (line !== '') { + if (accumulation) { accumulation += '\n' } + accumulation += line + } + } + } + }) + } + }) + } +}) diff --git a/tools/docgenerator.js b/tools/docgenerator.js index f18b2cc7b..34a22ce11 100644 --- a/tools/docgenerator.js +++ b/tools/docgenerator.js @@ -533,6 +533,81 @@ function cleanup (outputPath, outputRoot) { ]) } +/** + * Iterate over all source files and produce an object with information on + * all in-line documentation. + * @param {String[]} functionNames List with all functions exported from the main instance of mathjs + * @param {String} inputPath Path to location of source files + * @returns {object} docinfo + * Object whose keys are function names, and whose values are objects with + * keys name, category, fullpath, relativepath, doc, and issues + * giving the relevant information + */ +function collectDocs (functionNames, inputPath) { + const files = glob.sync(inputPath + '**/*.js') + + // generate path information for each of the files + const functions = {} // TODO: change to array + files.forEach(function (fullPath) { + const path = fullPath.split('/') + const name = path.pop().replace(/.js$/, '') + const functionIndex = path.indexOf('function') + let category + + // Note: determining whether a file is a function and what it's category + // is is a bit tricky and quite specific to the structure of the code, + // we reckon with some edge cases here. + if (path.indexOf('docs') === -1 && functionIndex !== -1) { + if (path.indexOf('expression') !== -1) { + category = 'expression' + } else if (/^.\/lib\/type\/[a-zA-Z0-9_]*\/function/.test(fullPath)) { + category = 'construction' + } else if (/^.\/lib\/core\/function/.test(fullPath)) { + category = 'core' + } else { + category = path[functionIndex + 1] + } + } else if (fullPath === './lib/cjs/expression/parse.js') { + // TODO: this is an ugly special case + category = 'expression' + } else if (path.join('/') === './lib/cjs/type') { + // for boolean.js, number.js, string.js + category = 'construction' + } + + if (functionNames.indexOf(name) === -1 || IGNORE_FUNCTIONS[name]) { + category = null + } + + if (category) { + functions[name] = { + name: name, + category: category, + fullPath: fullPath, + relativePath: fullPath.substring(inputPath.length) + } + } + }) + + // loop over all files, generate a doc for each of them + Object.keys(functions).forEach(name => { + const fn = functions[name] + const code = String(fs.readFileSync(fn.fullPath)) + + const isFunction = (functionNames.indexOf(name) !== -1) && !IGNORE_FUNCTIONS[name] + const doc = isFunction ? generateDoc(name, code) : null + + if (isFunction && doc) { + fn.doc = doc + fn.issues = validateDoc(doc) + } else { + // log('Ignoring', fn.fullPath) + delete functions[name] + } + }) + return functions +} + /** * Iterate over all source files and generate markdown documents for each of them * @param {String[]} functionNames List with all functions exported from the main instance of mathjs @@ -544,155 +619,91 @@ function iteratePath (functionNames, inputPath, outputPath, outputRoot) { if (!fs.existsSync(outputPath)) { mkdirp.sync(outputPath) } + const functions = collectDocs(functionNames, inputPath) + let issues = [] + for (const fn of Object.values(functions)) { + issues = issues.concat(fn.issues) + const markdown = generateMarkdown(fn.doc, functions) + fs.writeFileSync(outputPath + '/' + fn.name + '.md', markdown) + } - glob(inputPath + '**/*.js', null, function (err, files) { - if (err) { - console.error(err) - return - } - - // generate path information for each of the files - const functions = {} // TODO: change to array - - files.forEach(function (fullPath) { - const path = fullPath.split('/') - const name = path.pop().replace(/.js$/, '') - const functionIndex = path.indexOf('function') - let category - - // Note: determining whether a file is a function and what it's category - // is is a bit tricky and quite specific to the structure of the code, - // we reckon with some edge cases here. - if (path.indexOf('docs') === -1 && functionIndex !== -1) { - if (path.indexOf('expression') !== -1) { - category = 'expression' - } else if (/^.\/lib\/type\/[a-zA-Z0-9_]*\/function/.test(fullPath)) { - category = 'construction' - } else if (/^.\/lib\/core\/function/.test(fullPath)) { - category = 'core' - } else { - category = path[functionIndex + 1] - } - } else if (fullPath === './lib/cjs/expression/parse.js') { - // TODO: this is an ugly special case - category = 'expression' - } else if (path.join('/') === './lib/cjs/type') { - // for boolean.js, number.js, string.js - category = 'construction' - } - - if (functionNames.indexOf(name) === -1 || IGNORE_FUNCTIONS[name]) { - category = null - } - - if (category) { - functions[name] = { - name: name, - category: category, - fullPath: fullPath, - relativePath: fullPath.substring(inputPath.length) - } - } - }) - - // loop over all files, generate a doc for each of them - let issues = [] - Object.keys(functions).forEach(name => { - const fn = functions[name] - const code = String(fs.readFileSync(fn.fullPath)) - - const isFunction = (functionNames.indexOf(name) !== -1) && !IGNORE_FUNCTIONS[name] - const doc = isFunction ? generateDoc(name, code) : null - - if (isFunction && doc) { - fn.doc = doc - issues = issues.concat(validateDoc(doc)) - const markdown = generateMarkdown(doc, functions) - fs.writeFileSync(outputPath + '/' + fn.name + '.md', markdown) - } else { - // log('Ignoring', fn.fullPath) - delete functions[name] - } - }) - - /** - * Helper function to generate a markdown list entry for a function. - * Used to generate both alphabetical and categorical index pages. - * @param {string} name Function name + /** + * Helper function to generate a markdown list entry for a function. + * Used to generate both alphabetical and categorical index pages. + * @param {string} name Function name * @returns {string} Returns a markdown list entry */ - function functionEntry (name) { - const fn = functions[name] - let syntax = SYNTAX[name] || (fn.doc && fn.doc.syntax && fn.doc.syntax[0]) || name - syntax = syntax - // .replace(/^math\./, '') - .replace(/\s+\/\/.*$/, '') - .replace(/;$/, '') - if (syntax.length < 40) { - syntax = syntax.replace(/ /g, ' ') - } - - let description = '' - if (fn.doc.description) { - description = fn.doc.description.replace(/\n/g, ' ').split('.')[0] + '.' - } - - return '[' + syntax + '](functions/' + name + '.md) | ' + description + function functionEntry (name) { + const fn = functions[name] + let syntax = SYNTAX[name] || (fn.doc && fn.doc.syntax && fn.doc.syntax[0]) || name + syntax = syntax + // .replace(/^math\./, '') + .replace(/\s+\/\/.*$/, '') + .replace(/;$/, '') + if (syntax.length < 40) { + syntax = syntax.replace(/ /g, ' ') } - /** - * Change the first letter of the given string to upper case - * @param {string} text - */ - function toCapital (text) { - return text[0].toUpperCase() + text.slice(1) + let description = '' + if (fn.doc.description) { + description = fn.doc.description.replace(/\n/g, ' ').split('.')[0] + '.' } - const order = ['core', 'construction', 'expression'] // and then the rest - function categoryIndex (entry) { - const index = order.indexOf(entry) - return index === -1 ? Infinity : index - } - function compareAsc (a, b) { - return a > b ? 1 : (a < b ? -1 : 0) - } - function compareCategory (a, b) { - const indexA = categoryIndex(a) - const indexB = categoryIndex(b) - return (indexA > indexB) ? 1 : (indexA < indexB ? -1 : compareAsc(a, b)) - } + return '[' + syntax + '](functions/' + name + '.md) | ' + description + } - // generate categorical page with all functions - const categories = {} - Object.keys(functions).forEach(function (name) { - const fn = functions[name] - const category = categories[fn.category] - if (!category) { - categories[fn.category] = {} - } - categories[fn.category][name] = fn - }) - let categorical = '# Function reference\n\n' - categorical += Object.keys(categories).sort(compareCategory).map(function (category) { - const functions = categories[category] + /** + * Change the first letter of the given string to upper case + * @param {string} text + */ + function toCapital (text) { + return text[0].toUpperCase() + text.slice(1) + } - return '## ' + toCapital(category) + ' functions\n\n' + - 'Function | Description\n' + - '---- | -----------\n' + - Object.keys(functions).sort().map(functionEntry).join('\n') + '\n' - }).join('\n') - categorical += '\n\n\n\n' + const order = ['core', 'construction', 'expression'] // and then the rest + function categoryIndex (entry) { + const index = order.indexOf(entry) + return index === -1 ? Infinity : index + } + function compareAsc (a, b) { + return a > b ? 1 : (a < b ? -1 : 0) + } + function compareCategory (a, b) { + const indexA = categoryIndex(a) + const indexB = categoryIndex(b) + return (indexA > indexB) ? 1 : (indexA < indexB ? -1 : compareAsc(a, b)) + } - fs.writeFileSync(outputRoot + '/' + 'functions.md', categorical) - - // output all issues - if (issues.length) { - issues.forEach(function (issue) { - log('Warning: ' + issue) - }) - log(issues.length + ' warnings') + // generate categorical page with all functions + const categories = {} + Object.keys(functions).forEach(function (name) { + const fn = functions[name] + const category = categories[fn.category] + if (!category) { + categories[fn.category] = {} } + categories[fn.category][name] = fn }) + let categorical = '# Function reference\n\n' + categorical += Object.keys(categories).sort(compareCategory).map(function (category) { + const functions = categories[category] + + return '## ' + toCapital(category) + ' functions\n\n' + + 'Function | Description\n' + + '---- | -----------\n' + + Object.keys(functions).sort().map(functionEntry).join('\n') + '\n' + }).join('\n') + categorical += '\n\n\n\n' + + fs.writeFileSync(outputRoot + '/' + 'functions.md', categorical) + + // output all issues + if (issues.length) { + issues.forEach(function (issue) { + log('Warning: ' + issue) + }) + log(issues.length + ' warnings') + } } function findAll (text, regex) { @@ -711,6 +722,7 @@ function findAll (text, regex) { // exports exports.cleanup = cleanup +exports.collectDocs = collectDocs exports.iteratePath = iteratePath exports.generateDoc = generateDoc exports.validateDoc = validateDoc