From 941b65f68326b1e74374eb2040399773f67e09c8 Mon Sep 17 00:00:00 2001 From: Jonas Jenwald Date: Fri, 8 Jan 2021 17:12:58 +0100 Subject: [PATCH] Remove unncessary `CanvasFactory`/`CMapReaderFactory`/`FileReaderFactory` duplication in unit-tests Given that the API will now, after PR 12039, automatically pick the correct factories to use depending on the environment (browser vs. Node.js), we can utilize that in the unit-tests as well. This way we don't have to manually repeat the same initialization code in *multiple* unit-tests. *Note:* The *official* PDF.js API is defined in `src/pdf.js`, hence the new exports in `src/display/api.js` will not affect that. Also, updates the unit-test `FileReaderFactory` helpers similarily. *Drive-by change:* Fix the `CMapReaderFactory` usage in the annotation unit-tests, since the cache should only contain raw data and not a Promise. While this obviously works as-is, having unit-tests that "abuse" the intended data format can easily lead to unnecessary failures if changes are made to the relevant `src/core/` code. --- src/display/api.js | 2 ++ test/unit/annotation_spec.js | 33 +++++++----------------- test/unit/api_spec.js | 45 ++++++++++----------------------- test/unit/cmap_spec.js | 49 ++++++++++-------------------------- test/unit/custom_spec.js | 16 +++--------- test/unit/test_utils.js | 27 ++++++++++++-------- 6 files changed, 57 insertions(+), 115 deletions(-) diff --git a/src/display/api.js b/src/display/api.js index 40870c5c2..9bb3c6e52 100644 --- a/src/display/api.js +++ b/src/display/api.js @@ -3060,4 +3060,6 @@ export { setPDFNetworkStreamFactory, version, build, + DefaultCanvasFactory, + DefaultCMapReaderFactory, }; diff --git a/test/unit/annotation_spec.js b/test/unit/annotation_spec.js index 0abf022a5..18b4bc393 100644 --- a/test/unit/annotation_spec.js +++ b/test/unit/annotation_spec.js @@ -29,21 +29,14 @@ import { stringToBytes, stringToUTF8String, } from "../../src/shared/util.js"; -import { createIdFactory, XRefMock } from "./test_utils.js"; +import { CMAP_PARAMS, createIdFactory, XRefMock } from "./test_utils.js"; import { Dict, Name, Ref, RefSetCache } from "../../src/core/primitives.js"; import { Lexer, Parser } from "../../src/core/parser.js"; -import { DOMCMapReaderFactory } from "../../src/display/display_utils.js"; -import { isNodeJS } from "../../src/shared/is_node.js"; -import { NodeCMapReaderFactory } from "../../src/display/node_utils.js"; +import { DefaultCMapReaderFactory } from "../../src/display/api.js"; import { PartialEvaluator } from "../../src/core/evaluator.js"; import { StringStream } from "../../src/core/stream.js"; import { WorkerTask } from "../../src/core/worker.js"; -const cMapUrl = { - dom: "../../external/bcmaps/", - node: "./external/bcmaps/", -}; - describe("annotation", function () { class PDFManagerMock { constructor(params) { @@ -86,32 +79,24 @@ describe("annotation", function () { let pdfManagerMock, idFactoryMock, partialEvaluator; - beforeAll(function (done) { + beforeAll(async function (done) { pdfManagerMock = new PDFManagerMock({ docBaseUrl: null, }); - let CMapReaderFactory; - if (isNodeJS) { - CMapReaderFactory = new NodeCMapReaderFactory({ - baseUrl: cMapUrl.node, - isCompressed: true, - }); - } else { - CMapReaderFactory = new DOMCMapReaderFactory({ - baseUrl: cMapUrl.dom, - isCompressed: true, - }); - } + const CMapReaderFactory = new DefaultCMapReaderFactory({ + baseUrl: CMAP_PARAMS.cMapUrl, + isCompressed: CMAP_PARAMS.cMapPacked, + }); const builtInCMapCache = new Map(); builtInCMapCache.set( "UniJIS-UTF16-H", - CMapReaderFactory.fetch({ name: "UniJIS-UTF16-H" }) + await CMapReaderFactory.fetch({ name: "UniJIS-UTF16-H" }) ); builtInCMapCache.set( "Adobe-Japan1-UCS2", - CMapReaderFactory.fetch({ name: "Adobe-Japan1-UCS2" }) + await CMapReaderFactory.fetch({ name: "Adobe-Japan1-UCS2" }) ); idFactoryMock = createIdFactory(/* pageIndex = */ 0); diff --git a/test/unit/api_spec.js b/test/unit/api_spec.js index 5cb6dbc54..6c47f3244 100644 --- a/test/unit/api_spec.js +++ b/test/unit/api_spec.js @@ -15,8 +15,7 @@ import { buildGetDocumentParams, - DOMFileReaderFactory, - NodeFileReaderFactory, + DefaultFileReaderFactory, TEST_PDFS_PATH, } from "./test_utils.js"; import { @@ -32,23 +31,22 @@ import { StreamType, } from "../../src/shared/util.js"; import { - DOMCanvasFactory, - RenderingCancelledException, - StatTimer, -} from "../../src/display/display_utils.js"; -import { + DefaultCanvasFactory, getDocument, PDFDataRangeTransport, PDFDocumentProxy, PDFPageProxy, PDFWorker, } from "../../src/display/api.js"; +import { + RenderingCancelledException, + StatTimer, +} from "../../src/display/display_utils.js"; import { AutoPrintRegExp } from "../../web/ui_utils.js"; import { GlobalImageCache } from "../../src/core/image_utils.js"; import { GlobalWorkerOptions } from "../../src/display/worker_options.js"; import { isNodeJS } from "../../src/shared/is_node.js"; import { Metadata } from "../../src/display/metadata.js"; -import { NodeCanvasFactory } from "../../src/display/node_utils.js"; describe("api", function () { const basicApiFileName = "basicapi.pdf"; @@ -58,11 +56,7 @@ describe("api", function () { let CanvasFactory; beforeAll(function (done) { - if (isNodeJS) { - CanvasFactory = new NodeCanvasFactory(); - } else { - CanvasFactory = new DOMCanvasFactory(); - } + CanvasFactory = new DefaultCanvasFactory(); done(); }); @@ -132,16 +126,9 @@ describe("api", function () { .catch(done.fail); }); it("creates pdf doc from typed array", function (done) { - let typedArrayPdfPromise; - if (isNodeJS) { - typedArrayPdfPromise = NodeFileReaderFactory.fetch({ - path: TEST_PDFS_PATH.node + basicApiFileName, - }); - } else { - typedArrayPdfPromise = DOMFileReaderFactory.fetch({ - path: TEST_PDFS_PATH.dom + basicApiFileName, - }); - } + const typedArrayPdfPromise = DefaultFileReaderFactory.fetch({ + path: TEST_PDFS_PATH + basicApiFileName, + }); typedArrayPdfPromise .then(typedArrayPdf => { @@ -2197,15 +2184,9 @@ describe("api", function () { beforeAll(function (done) { const fileName = "tracemonkey.pdf"; - if (isNodeJS) { - dataPromise = NodeFileReaderFactory.fetch({ - path: TEST_PDFS_PATH.node + fileName, - }); - } else { - dataPromise = DOMFileReaderFactory.fetch({ - path: TEST_PDFS_PATH.dom + fileName, - }); - } + dataPromise = DefaultFileReaderFactory.fetch({ + path: TEST_PDFS_PATH + fileName, + }); done(); }); diff --git a/test/unit/cmap_spec.js b/test/unit/cmap_spec.js index 119a7aae1..77df4f031 100644 --- a/test/unit/cmap_spec.js +++ b/test/unit/cmap_spec.js @@ -14,35 +14,20 @@ */ import { CMap, CMapFactory, IdentityCMap } from "../../src/core/cmap.js"; -import { DOMCMapReaderFactory } from "../../src/display/display_utils.js"; -import { isNodeJS } from "../../src/shared/is_node.js"; +import { CMAP_PARAMS } from "./test_utils.js"; +import { DefaultCMapReaderFactory } from "../../src/display/api.js"; import { Name } from "../../src/core/primitives.js"; -import { NodeCMapReaderFactory } from "../../src/display/node_utils.js"; import { StringStream } from "../../src/core/stream.js"; -const cMapUrl = { - dom: "../../external/bcmaps/", - node: "./external/bcmaps/", -}; -const cMapPacked = true; - describe("cmap", function () { let fetchBuiltInCMap; beforeAll(function (done) { // Allow CMap testing in Node.js, e.g. for Travis. - let CMapReaderFactory; - if (isNodeJS) { - CMapReaderFactory = new NodeCMapReaderFactory({ - baseUrl: cMapUrl.node, - isCompressed: cMapPacked, - }); - } else { - CMapReaderFactory = new DOMCMapReaderFactory({ - baseUrl: cMapUrl.dom, - isCompressed: cMapPacked, - }); - } + const CMapReaderFactory = new DefaultCMapReaderFactory({ + baseUrl: CMAP_PARAMS.cMapUrl, + isCompressed: CMAP_PARAMS.cMapPacked, + }); fetchBuiltInCMap = function (name) { return CMapReaderFactory.fetch({ @@ -298,9 +283,8 @@ describe("cmap", function () { it("attempts to load a built-in CMap without the necessary API parameters", function (done) { function tmpFetchBuiltInCMap(name) { - const CMapReaderFactory = isNodeJS - ? new NodeCMapReaderFactory({}) - : new DOMCMapReaderFactory({}); + const CMapReaderFactory = new DefaultCMapReaderFactory({}); + return CMapReaderFactory.fetch({ name, }); @@ -328,18 +312,11 @@ describe("cmap", function () { it("attempts to load a built-in CMap with inconsistent API parameters", function (done) { function tmpFetchBuiltInCMap(name) { - let CMapReaderFactory; - if (isNodeJS) { - CMapReaderFactory = new NodeCMapReaderFactory({ - baseUrl: cMapUrl.node, - isCompressed: false, - }); - } else { - CMapReaderFactory = new DOMCMapReaderFactory({ - baseUrl: cMapUrl.dom, - isCompressed: false, - }); - } + const CMapReaderFactory = new DefaultCMapReaderFactory({ + baseUrl: CMAP_PARAMS.cMapUrl, + isCompressed: false, + }); + return CMapReaderFactory.fetch({ name, }); diff --git a/test/unit/custom_spec.js b/test/unit/custom_spec.js index 8618b4fc4..aa393af7c 100644 --- a/test/unit/custom_spec.js +++ b/test/unit/custom_spec.js @@ -13,11 +13,8 @@ * limitations under the License. */ +import { DefaultCanvasFactory, getDocument } from "../../src/display/api.js"; import { buildGetDocumentParams } from "./test_utils.js"; -import { DOMCanvasFactory } from "../../src/display/display_utils.js"; -import { getDocument } from "../../src/display/api.js"; -import { isNodeJS } from "../../src/shared/is_node.js"; -import { NodeCanvasFactory } from "../../src/display/node_utils.js"; function getTopLeftPixel(canvasContext) { const imgData = canvasContext.getImageData(0, 0, 1, 1); @@ -39,11 +36,8 @@ describe("custom canvas rendering", function () { let page; beforeAll(function (done) { - if (isNodeJS) { - CanvasFactory = new NodeCanvasFactory(); - } else { - CanvasFactory = new DOMCanvasFactory(); - } + CanvasFactory = new DefaultCanvasFactory(); + loadingTask = getDocument(transparentGetDocumentParams); loadingTask.promise .then(function (doc) { @@ -156,10 +150,8 @@ describe("custom ownerDocument", function () { getElementsByTagName: () => [{ appendChild: () => {} }], }, }; + const CanvasFactory = new DefaultCanvasFactory({ ownerDocument }); - const CanvasFactory = isNodeJS - ? new NodeCanvasFactory() - : new DOMCanvasFactory({ ownerDocument }); return { elements, ownerDocument, diff --git a/test/unit/test_utils.js b/test/unit/test_utils.js index 635b78c93..50b6c1428 100644 --- a/test/unit/test_utils.js +++ b/test/unit/test_utils.js @@ -19,6 +19,13 @@ import { assert } from "../../src/shared/util.js"; import { isNodeJS } from "../../src/shared/is_node.js"; import { StringStream } from "../../src/core/stream.js"; +const TEST_PDFS_PATH = isNodeJS ? "./test/pdfs/" : "../pdfs/"; + +const CMAP_PARAMS = { + cMapUrl: isNodeJS ? "./external/bcmaps/" : "../../external/bcmaps/", + cMapPacked: true, +}; + class DOMFileReaderFactory { static async fetch(params) { const response = await fetch(params.path); @@ -45,18 +52,16 @@ class NodeFileReaderFactory { } } -const TEST_PDFS_PATH = { - dom: "../pdfs/", - node: "./test/pdfs/", -}; +const DefaultFileReaderFactory = isNodeJS + ? NodeFileReaderFactory + : DOMFileReaderFactory; function buildGetDocumentParams(filename, options) { const params = Object.create(null); - if (isNodeJS) { - params.url = TEST_PDFS_PATH.node + filename; - } else { - params.url = new URL(TEST_PDFS_PATH.dom + filename, window.location).href; - } + params.url = isNodeJS + ? TEST_PDFS_PATH + filename + : new URL(TEST_PDFS_PATH + filename, window.location).href; + for (const option in options) { params[option] = options[option]; } @@ -136,11 +141,11 @@ function isEmptyObj(obj) { } export { - DOMFileReaderFactory, - NodeFileReaderFactory, + DefaultFileReaderFactory, XRefMock, buildGetDocumentParams, TEST_PDFS_PATH, + CMAP_PARAMS, createIdFactory, isEmptyObj, };