From 7461fec0120fabd1df3e2aeaa47ac0bd50a974f8 Mon Sep 17 00:00:00 2001 From: Romain Deltour Date: Mon, 2 Oct 2017 10:57:39 +0200 Subject: [PATCH] fix: enable successive calls in the same process The `report` module was using a shared report object which was mutated all along the checking process. When calling `ace.js` multiple times sequentially, that shared report object wasn't cleaned and the later runs inherited the assertions from the previous runs. The `report` module now exports a single `EPUB` class, with no shared state. The report is passed along the checking process, up until the `checkSingle` method. After all documents are checked, we consolidate the individual results and add them to the given report object. Also add some integration tests for checking the JSON report. --- src/checker/checker-nightmare.js | 11 ++-- src/checker/checker.js | 23 ++++--- src/core/ace.js | 36 ++++------ src/report/json-report-builder.js | 1 - src/report/report.js | 99 +++++++++++++++------------- tests/__tests__/report_files.test.js | 35 ++++------ tests/__tests__/report_json.test.js | 55 ++++++++++++++++ tests/runAceJS.js | 21 ++++++ 8 files changed, 171 insertions(+), 110 deletions(-) create mode 100644 tests/__tests__/report_json.test.js create mode 100644 tests/runAceJS.js diff --git a/src/checker/checker-nightmare.js b/src/checker/checker-nightmare.js index 30f44452..ca9d2bed 100644 --- a/src/checker/checker-nightmare.js +++ b/src/checker/checker-nightmare.js @@ -3,7 +3,6 @@ const fs = require('fs'); const path = require('path'); const Nightmare = require('nightmare'); -const report = require('../report/report.js'); const axe2ace = require('../report/axe2ace.js'); const winston = require('winston'); @@ -58,12 +57,10 @@ function checkSingle(spineItem, epub, nightmare) { const results = json; results.assertions = (json.axe != null) ? axe2ace.axe2ace(spineItem, json.axe) : []; delete results.axe; - var numIssues = 0; - if (results.assertions != null) { - numIssues = results.assertions.assertions.length; - report.addContentDocAssertion(results.assertions); - } - winston.info(`- ${numIssues} issues found`); + winston.info(`- ${ + (results.assertions == null) + ? 'No' + : results.assertions.assertions.length} issues found`); if (results.data != null && results.data.images != null) { results.data.images.forEach((img) => { const imageFullPath = path.resolve(path.dirname(spineItem.filepath), img.path); diff --git a/src/checker/checker.js b/src/checker/checker.js index 355f1eb0..06403548 100644 --- a/src/checker/checker.js +++ b/src/checker/checker.js @@ -1,24 +1,31 @@ 'use strict'; const checker = require('./checker-nightmare.js'); -const report = require('../report/report.js'); const winston = require('winston'); -function finalize(results) { +function finalize(results, report) { + // Copy assertions + results + .filter(res => res.assertions != null) + .forEach(res => report.addContentDocAssertion(res.assertions)); // Get a flat array of all the headings in the documents - const headings = [].concat(...results.map(single => single.outlines.headings)); + const headings = [] + .concat(...results.map(docResult => docResult.outlines.headings)) + .filter(e => e !== undefined); report.addHeadings(headings); // Aggregated array of the HTML outlines - const htmlOutlines = results.map(single => single.outlines.html); + const htmlOutlines = results.map(docResult => docResult.outlines.html); report.addHTMLOutlines(htmlOutlines); // Get a flat array of the extracted images - const images = [].concat(...results.map(single => single.data.images)); + const images = [] + .concat(...results.map(docResult => docResult.data.images)) + .filter(e => e !== undefined); report.addImages(images); - return results; + return report; } -module.exports.check = function check(epub) { +module.exports.check = function check(epub, report) { winston.info('Checking documents...'); return checker.check(epub) - .then(results => finalize(results)); + .then(results => finalize(results, report)); }; diff --git a/src/core/ace.js b/src/core/ace.js index 93fa9be7..6fb23cc8 100644 --- a/src/core/ace.js +++ b/src/core/ace.js @@ -6,7 +6,7 @@ const tmp = require('tmp'); const checker = require('../checker/checker.js'); const EPUB = require('../epub/epub.js'); -const report = require('../report/report.js'); +const Report = require('../report/report.js'); const winston = require('winston'); tmp.setGracefulCleanup(); @@ -53,22 +53,21 @@ module.exports = function ace(epubPath, options) { epub.extract() .then(() => epub.parse()) // initialize the report - .then(() => report.initialize(epub)) + .then(() => new Report(epub)) // Report the Nav Doc - .then(() => report.addEPUBNav(epub.navDoc)) + .then(report => report.addEPUBNav(epub.navDoc)) // Check each Content Doc - .then(() => checker.check(epub)) + .then(report => checker.check(epub, report)) // Process the Results - .then(() => { + .then((report) => { if (options.outdir === undefined) { - winston.info(JSON.stringify(report.getJsonReport(), null, ' ')); - } else { - return Promise.all([ - report.copyData(options.outdir), - report.saveJson(options.outdir), - report.saveHtml(options.outdir), - ]); + return winston.info(JSON.stringify(report.json, null, ' ')); } + return Promise.all([ + report.copyData(options.outdir), + report.saveJson(options.outdir), + report.saveHtml(options.outdir), + ]); }) .then(() => { winston.info('Done.'); @@ -80,17 +79,4 @@ module.exports = function ace(epubPath, options) { reject(jobId); }); }); - // // Alternatively: use reduce to process results progressively ? - // new EPUB(epubPath).extract() - // .then(epub => epub.getContentDocsPaths()) - // .then(paths => paths.map(path => `file://${path}`)) - // .then(urls => - // urls.reduce((sequence, url) => - // sequence.then(() => check(url)).then((result) => { - // console.log(`do sth with ${result}`); - // return result; - // }), Promise.resolve())) - // .then(...); - // getJSON('story.json').then(function(story) { - // addHtmlToPage(story.heading); }; diff --git a/src/report/json-report-builder.js b/src/report/json-report-builder.js index e085967b..823bb53f 100644 --- a/src/report/json-report-builder.js +++ b/src/report/json-report-builder.js @@ -53,7 +53,6 @@ function withAssertion(obj, assertion) { } obj["assertions"].push(assertion); withResult(obj, calculateResult(obj.assertions)); - return obj; } function withResult(obj, result) { diff --git a/src/report/report.js b/src/report/report.js index 9f1e5e75..af2a6938 100644 --- a/src/report/report.js +++ b/src/report/report.js @@ -23,8 +23,6 @@ const winston = require('winston'); const reportBuilder = require('./json-report-builder.js'); const a11yMetaChecker = require("./analyze-a11y-metadata.js"); -const report = exports; -const jsonReport = new reportBuilder.Report(); function headingsToOutline(headings) { const result = []; @@ -57,45 +55,51 @@ function aggregateHTMLOutlines(outlines) { return `
  1. ${outlines.join('
  2. ')}
`; } +module.exports = class Report { -report.initialize = function initialize(epub) { - jsonReport.withTitle("ACE Report") + + constructor(epub) { + this.json = new reportBuilder.Report(); + this.json.withTitle("ACE Report") .withDescription("Accessibility Checker Report" ) .withTestSubject(epub.path, "", "", epub.metadata); - jsonReport.withA11yMeta(a11yMetaChecker.analyze(epub.metadata)); + this.json.withA11yMeta(a11yMetaChecker.analyze(epub.metadata)); + } -} -report.addContentDocAssertion = function addContentDocAssertion(assertion) { - jsonReport.withAssertion(assertion); -} -report.addOutline = function addOutline(outline) { - jsonReport.withHOutline(outline); -} -report.addHeadings = function addHeadings(headings) { - jsonReport.withHeadingsOutline(headingsToOutline(headings)); -} -report.addHTMLOutlines = function addHTMLOutlines(outlines) { - jsonReport.withHTMLOutline(aggregateHTMLOutlines(outlines)); -} -report.addEPUBNav = function addEPUBNav(navDoc) { - jsonReport.withEPUBOutline(navDoc.tocHTML); -} -report.addImages = function addHImages(images) { - jsonReport.withImages(images); -} -report.getJsonReport = function getJsonReport() { - return jsonReport; -} -report.copyData = function copyData(outdir) { + addContentDocAssertion(assertion) { + this.json.withAssertion(assertion); + return this; + } + addOutline(outline) { + this.json.withHOutline(outline); + return this; + } + addHeadings(headings) { + this.json.withHeadingsOutline(headingsToOutline(headings)); + return this; + } + addHTMLOutlines(outlines) { + this.json.withHTMLOutline(aggregateHTMLOutlines(outlines)); + return this; + } + addEPUBNav(navDoc) { + this.json.withEPUBOutline(navDoc.tocHTML); + return this; + } + addImages(images) { + this.json.withImages(images); + return this; + } + copyData(outdir) { winston.info("Copying data"); - if (jsonReport.data === null) return; + if (this.json.data === null) return; return fs.pathExists(outdir) .then((exists) => { if (!exists) fs.mkdirSync(outdir); }) .then(() => { - if (jsonReport.data.images != null) { - jsonReport.data.images.forEach((img) => { + if (this.json.data.images != null) { + this.json.data.images.forEach((img) => { const fromPath = img.filepath; const toPath = path.join(outdir, 'data', img.path); delete img.filepath; @@ -105,31 +109,32 @@ report.copyData = function copyData(outdir) { }); } }); -} -report.saveJson = function saveJson(outdir) { + } + saveJson(outdir) { winston.info("Saving JSON report"); return fs.pathExists(outdir) .then((exists) => { if (!exists) fs.mkdirSync(outdir); }) .then(() => { - const aceReport = JSON.stringify(jsonReport, null, ' '); + const aceReport = JSON.stringify(this.json, null, ' '); return aceReport; }) .then((aceReport) => Promise.all([ fs.writeFile(path.join(outdir, 'ace.json'), aceReport, 'UTF-8'), ])); -} -report.saveHtml = function saveHtml(outdir) { - winston.info("Saving HTML report"); - // create a js file that the html report uses as its data source - const aceReport = JSON.stringify(jsonReport, null, ' '); - const js = "const aceReportData = " + aceReport + ";"; - - // copy report.html and the contents of /js and /css to the outdir - return fs.copy(path.join(__dirname, 'resources/report.html'), path.join(outdir, "report.html")) - //.then(() => fs.copy(path.join(__dirname, './resources/css/'), path.join(outdir, "css/"))) - .then(() => fs.copy(path.join(__dirname, './resources/js/'), path.join(outdir, "js/"))) - .then(() => fs.writeFile(path.join(outdir, 'js/', 'aceReportData.js'), js, 'UTF-8')) - .catch(err => winston.error(err)); + } + saveHtml(outdir) { + winston.info("Saving HTML report"); + // create a js file that the html report uses as its data source + const aceReport = JSON.stringify(this.json, null, ' '); + const js = "const aceReportData = " + aceReport + ";"; + + // copy report.html and the contents of /js and /css to the outdir + return fs.copy(path.join(__dirname, 'resources/report.html'), path.join(outdir, "report.html")) + //.then(() => fs.copy(path.join(__dirname, './resources/css/'), path.join(outdir, "css/"))) + .then(() => fs.copy(path.join(__dirname, './resources/js/'), path.join(outdir, "js/"))) + .then(() => fs.writeFile(path.join(outdir, 'js/', 'aceReportData.js'), js, 'UTF-8')) + .catch(err => winston.error(err)); + } } diff --git a/tests/__tests__/report_files.test.js b/tests/__tests__/report_files.test.js index b36af6ba..bab687c4 100644 --- a/tests/__tests__/report_files.test.js +++ b/tests/__tests__/report_files.test.js @@ -4,12 +4,19 @@ const fs = require('fs'); const path = require('path'); const tmp = require('tmp'); -const ace = require('../../src/core/ace.js'); +const runAce = require('../runAceJS'); + +tmp.setGracefulCleanup(); let outdir; let tmpdir; -tmp.setGracefulCleanup(); +const ace = function ace(epub, options = {}) { + return runAce(epub, Object.assign({ + outdir: outdir.name, + tmp: tmpdir.name, + }, options)); +}; beforeEach(() => { outdir = tmp.dirSync({ prefix: 'ace_out_', unsafeCleanup: true }); @@ -21,31 +28,15 @@ afterEach(() => { tmpdir.removeCallback(); }); -function runAce(epub, { - cwd = process.cwd(), - outpath = outdir.name, - tmppath = tmpdir.name, - verbose = false, - silent = true, - } = {}) { - return ace(epub, { - cwd, - outdir: outpath, - tmpdir: tmppath, - verbose, - silent, - }); -} - test('unexisting EPUB fails with an error', () => { expect.assertions(1); - return expect(runAce('noepub')) + return expect(ace('noepub')) .rejects.toMatch(''); }); test('packaged EPUB with absolute path', async () => { expect.assertions(1); - await runAce(path.join(__dirname, '../data/base-epub-30.epub')); + await ace(path.join(__dirname, '../data/base-epub-30.epub')); expect(fs.existsSync(path.join(outdir.name, 'report.html'))).toBeTruthy(); }); @@ -57,7 +48,7 @@ test('packaged EPUB with relative path', async () => { test('expanded EPUB with absolute path', async () => { expect.assertions(1); - await runAce(path.join(__dirname, '../data/base-epub-30')); + await ace(path.join(__dirname, '../data/base-epub-30')); expect(fs.existsSync(path.join(outdir.name, 'report.html'))).toBeTruthy(); }); @@ -72,7 +63,7 @@ test('files don’t leak outside the report dir', async () => { const outpath = path.join(outdir.name, 'report'); fs.mkdirSync(outpath); expect.assertions(2); - await runAce(path.join(__dirname, '../data/issue33'), { outpath }); + await ace(path.join(__dirname, '../data/issue33'), { outdir: outpath }); expect(fs.existsSync(path.join(outpath, 'report.html'))).toBeTruthy(); expect(fs.existsSync(path.join(outpath, 'data/EPUB/images/img_001.jpg'))).toBeTruthy(); }); diff --git a/tests/__tests__/report_json.test.js b/tests/__tests__/report_json.test.js new file mode 100644 index 00000000..6ccc1bc7 --- /dev/null +++ b/tests/__tests__/report_json.test.js @@ -0,0 +1,55 @@ +'use strict'; + +const fs = require('fs'); +const path = require('path'); +const tmp = require('tmp'); + +const runAce = require('../runAceJS'); + +tmp.setGracefulCleanup(); + +let outdir; +let tmpdir; +let reportPath; + +beforeEach(() => { + outdir = tmp.dirSync({ prefix: 'ace_out_', unsafeCleanup: true }); + tmpdir = tmp.dirSync({ prefix: 'ace_tmp_', unsafeCleanup: true }); + reportPath = path.join(outdir.name, 'ace.json'); +}); + +afterEach(() => { + outdir.removeCallback(); + tmpdir.removeCallback(); +}); + + +function ace(epub, options = {}) { + return runAce(epub, Object.assign({ + outdir: outdir.name, + tmp: tmpdir.name, + }, options)) + .then(() => { + expect(fs.existsSync(reportPath)).toBeTruthy(); + return JSON.parse(fs.readFileSync(reportPath, 'utf8')); + }) + .catch(err => console.log(err)); +} + +test('report exists and is well-formed', async () => { + const report = await ace(path.join(__dirname, '../data/base-epub-30')); + expect(report.outlines).toBeDefined(); + expect(report.assertions).toBeDefined(); + expect(report.data).toBeDefined(); + expect(report['earl:result']).toBeDefined(); +}); + +test('with no issues', async () => { + const report = await ace(path.join(__dirname, '../data/base-epub-30')); + expect(report['earl:result']).toBeDefined(); + expect(report['earl:result']).toEqual({ 'earl:outcome': 'pass' }); + expect(report.assertions).toMatchObject([{ + '@type': 'earl:assertion', + assertions: [], + }]); +}); diff --git a/tests/runAceJS.js b/tests/runAceJS.js new file mode 100644 index 00000000..54ed569b --- /dev/null +++ b/tests/runAceJS.js @@ -0,0 +1,21 @@ +'use strict'; + +const ace = require('../src/core/ace'); + +function runAce(epub, { + cwd = process.cwd(), + outdir, + tmpdir, + verbose = false, + silent = true, + } = {}) { + return ace(epub, { + cwd, + outdir, + tmpdir, + verbose, + silent, + }); +} + +module.exports = runAce;