From c83ad0128d4e04bd5f86c168e9d12eb3041d167f Mon Sep 17 00:00:00 2001 From: Robert Knight Date: Wed, 5 May 2021 14:27:29 +0100 Subject: [PATCH 1/4] Refactor test setup in `PDFMetadata` tests Refactor the setup steps in PDFMetadata tests to make it easier to customize the PDF metadata exposed by the fake PDF.js environment. Instead of creating a fake `PDFViewerApplication` and `PDFMetadata` instance before each test, provide a helper function that creates both using the provided metadata. --- .../integrations/test/pdf-metadata-test.js | 40 ++++++++----------- 1 file changed, 17 insertions(+), 23 deletions(-) diff --git a/src/annotator/integrations/test/pdf-metadata-test.js b/src/annotator/integrations/test/pdf-metadata-test.js index 77ed8f3d85a..e963de6aed0 100644 --- a/src/annotator/integrations/test/pdf-metadata-test.js +++ b/src/annotator/integrations/test/pdf-metadata-test.js @@ -209,51 +209,48 @@ describe('PDFMetadata', function () { }); describe('metadata sources', function () { - let pdfMetadata; - const fakePDFViewerApplication = new FakePDFViewerApplication(); - fakePDFViewerApplication.completeInit(); - fakePDFViewerApplication.finishLoading({ + const testMetadata = { fingerprint: 'fakeFingerprint', title: 'fakeTitle', metadata: { 'dc:title': 'dcFakeTitle', }, url: 'http://fake.com/', - }); + }; - beforeEach(function () { - pdfMetadata = new PDFMetadata(fakePDFViewerApplication); - }); + function createPDFMetadata(metadata = testMetadata) { + const fakePDFViewerApplication = new FakePDFViewerApplication(); + fakePDFViewerApplication.completeInit(); + fakePDFViewerApplication.finishLoading(metadata); + return { + fakePDFViewerApplication, + pdfMetadata: new PDFMetadata(fakePDFViewerApplication), + }; + } describe('#getUri', function () { it('returns the non-file URI', function () { + const { pdfMetadata } = createPDFMetadata(); return pdfMetadata.getUri().then(function (uri) { assert.equal(uri, 'http://fake.com/'); }); }); it('returns the fingerprint as a URN when the PDF URL is a local file', function () { - const fakePDFViewerApplication = new FakePDFViewerApplication(); - fakePDFViewerApplication.completeInit(); - fakePDFViewerApplication.finishLoading({ + const { pdfMetadata } = createPDFMetadata({ url: 'file:///test.pdf', fingerprint: 'fakeFingerprint', }); - const pdfMetadata = new PDFMetadata(fakePDFViewerApplication); - return pdfMetadata.getUri().then(function (uri) { assert.equal(uri, 'urn:x-pdf:fakeFingerprint'); }); }); it('resolves relative URLs', () => { - const fakePDFViewerApplication = new FakePDFViewerApplication(); - fakePDFViewerApplication.completeInit(); - fakePDFViewerApplication.finishLoading({ + const { fakePDFViewerApplication, pdfMetadata } = createPDFMetadata({ url: 'index.php?action=download&file_id=wibble', fingerprint: 'fakeFingerprint', }); - const pdfMetadata = new PDFMetadata(fakePDFViewerApplication); return pdfMetadata.getUri().then(uri => { const expected = new URL( @@ -267,6 +264,7 @@ describe('PDFMetadata', function () { describe('#getMetadata', function () { it('gets the title from the dc:title field', function () { + const { fakePDFViewerApplication, pdfMetadata } = createPDFMetadata(); const expectedMetadata = { title: 'dcFakeTitle', link: [ @@ -285,6 +283,7 @@ describe('PDFMetadata', function () { }); it('gets the title from the documentInfo.Title field', function () { + const { fakePDFViewerApplication, pdfMetadata } = createPDFMetadata(); const expectedMetadata = { title: fakePDFViewerApplication.documentInfo.Title, link: [ @@ -305,10 +304,7 @@ describe('PDFMetadata', function () { }); it('does not save file:// URLs in document metadata', function () { - let pdfMetadata; - const fakePDFViewerApplication = new FakePDFViewerApplication(); - fakePDFViewerApplication.completeInit(); - fakePDFViewerApplication.finishLoading({ + const { fakePDFViewerApplication, pdfMetadata } = createPDFMetadata({ fingerprint: 'fakeFingerprint', url: 'file://fake.pdf', }); @@ -321,8 +317,6 @@ describe('PDFMetadata', function () { ], }; - pdfMetadata = new PDFMetadata(fakePDFViewerApplication); - return pdfMetadata.getMetadata().then(function (actualMetadata) { assert.equal(actualMetadata.link.length, 1); assert.equal( From 6bf4c7555319dedb3509a47dfc9b911d08de832c Mon Sep 17 00:00:00 2001 From: Robert Knight Date: Wed, 5 May 2021 14:47:14 +0100 Subject: [PATCH 2/4] Refactor `getMetadata` tests - Convert promise chains to async/await - Add missing tests to check that the PDF URL and fingerprint URL appear in the object returned by `getMetadata`. These were previously tested indirectly in other tests. - Change tests to only check the specific properties of the returned object that are of interest. This reduces the changes needed when tests change unrelated parts of the output. --- .../integrations/test/pdf-metadata-test.js | 190 ++++++++---------- 1 file changed, 89 insertions(+), 101 deletions(-) diff --git a/src/annotator/integrations/test/pdf-metadata-test.js b/src/annotator/integrations/test/pdf-metadata-test.js index e963de6aed0..1fecdffe9e5 100644 --- a/src/annotator/integrations/test/pdf-metadata-test.js +++ b/src/annotator/integrations/test/pdf-metadata-test.js @@ -208,123 +208,111 @@ describe('PDFMetadata', function () { }); }); - describe('metadata sources', function () { - const testMetadata = { - fingerprint: 'fakeFingerprint', - title: 'fakeTitle', - metadata: { - 'dc:title': 'dcFakeTitle', - }, - url: 'http://fake.com/', + const testMetadata = { + fingerprint: 'fakeFingerprint', + title: 'fakeTitle', + metadata: { + 'dc:title': 'dcFakeTitle', + }, + url: 'http://fake.com/', + }; + + function createPDFMetadata(metadata = testMetadata) { + const fakePDFViewerApplication = new FakePDFViewerApplication(); + fakePDFViewerApplication.completeInit(); + fakePDFViewerApplication.finishLoading(metadata); + return { + fakePDFViewerApplication, + pdfMetadata: new PDFMetadata(fakePDFViewerApplication), }; + } - function createPDFMetadata(metadata = testMetadata) { - const fakePDFViewerApplication = new FakePDFViewerApplication(); - fakePDFViewerApplication.completeInit(); - fakePDFViewerApplication.finishLoading(metadata); - return { - fakePDFViewerApplication, - pdfMetadata: new PDFMetadata(fakePDFViewerApplication), - }; - } + describe('#getUri', function () { + it('returns the non-file URI', function () { + const { pdfMetadata } = createPDFMetadata(); + return pdfMetadata.getUri().then(function (uri) { + assert.equal(uri, 'http://fake.com/'); + }); + }); - describe('#getUri', function () { - it('returns the non-file URI', function () { - const { pdfMetadata } = createPDFMetadata(); - return pdfMetadata.getUri().then(function (uri) { - assert.equal(uri, 'http://fake.com/'); - }); + it('returns the fingerprint as a URN when the PDF URL is a local file', function () { + const { pdfMetadata } = createPDFMetadata({ + url: 'file:///test.pdf', + fingerprint: 'fakeFingerprint', + }); + return pdfMetadata.getUri().then(function (uri) { + assert.equal(uri, 'urn:x-pdf:fakeFingerprint'); }); + }); - it('returns the fingerprint as a URN when the PDF URL is a local file', function () { - const { pdfMetadata } = createPDFMetadata({ - url: 'file:///test.pdf', - fingerprint: 'fakeFingerprint', - }); - return pdfMetadata.getUri().then(function (uri) { - assert.equal(uri, 'urn:x-pdf:fakeFingerprint'); - }); + it('resolves relative URLs', () => { + const { fakePDFViewerApplication, pdfMetadata } = createPDFMetadata({ + url: 'index.php?action=download&file_id=wibble', + fingerprint: 'fakeFingerprint', }); - it('resolves relative URLs', () => { - const { fakePDFViewerApplication, pdfMetadata } = createPDFMetadata({ - url: 'index.php?action=download&file_id=wibble', - fingerprint: 'fakeFingerprint', - }); + return pdfMetadata.getUri().then(uri => { + const expected = new URL( + fakePDFViewerApplication.url, + document.location.href + ).toString(); + assert.equal(uri, expected); + }); + }); + }); - return pdfMetadata.getUri().then(uri => { - const expected = new URL( - fakePDFViewerApplication.url, - document.location.href - ).toString(); - assert.equal(uri, expected); - }); + describe('#getMetadata', () => { + it('returns the document fingerprint in the `documentFingerprint` property', async () => { + const { pdfMetadata } = createPDFMetadata(); + const metadata = await pdfMetadata.getMetadata(); + assert.equal(metadata.documentFingerprint, testMetadata.fingerprint); + }); + + it('returns the PDF URL in the `links` array', async () => { + const { pdfMetadata } = createPDFMetadata(); + const metadata = await pdfMetadata.getMetadata(); + assert.deepInclude(metadata.link, { + href: testMetadata.url, }); }); - describe('#getMetadata', function () { - it('gets the title from the dc:title field', function () { - const { fakePDFViewerApplication, pdfMetadata } = createPDFMetadata(); - const expectedMetadata = { - title: 'dcFakeTitle', - link: [ - { - href: - 'urn:x-pdf:' + fakePDFViewerApplication.pdfDocument.fingerprint, - }, - { href: fakePDFViewerApplication.url }, - ], - documentFingerprint: fakePDFViewerApplication.pdfDocument.fingerprint, - }; - - return pdfMetadata.getMetadata().then(function (actualMetadata) { - assert.deepEqual(actualMetadata, expectedMetadata); - }); + it('returns the document fingerprint in the `links` array', async () => { + const { pdfMetadata } = createPDFMetadata(); + const metadata = await pdfMetadata.getMetadata(); + assert.deepInclude(metadata.link, { + href: `urn:x-pdf:${testMetadata.fingerprint}`, }); + }); - it('gets the title from the documentInfo.Title field', function () { - const { fakePDFViewerApplication, pdfMetadata } = createPDFMetadata(); - const expectedMetadata = { - title: fakePDFViewerApplication.documentInfo.Title, - link: [ - { - href: - 'urn:x-pdf:' + fakePDFViewerApplication.pdfDocument.fingerprint, - }, - { href: fakePDFViewerApplication.url }, - ], - documentFingerprint: fakePDFViewerApplication.pdfDocument.fingerprint, - }; - - fakePDFViewerApplication.metadata.has = sinon.stub().returns(false); - - return pdfMetadata.getMetadata().then(function (actualMetadata) { - assert.deepEqual(actualMetadata, expectedMetadata); - }); + it('does not return file:// URLs in `links` array', async () => { + const { pdfMetadata } = createPDFMetadata({ + fingerprint: 'fakeFingerprint', + url: 'file://fake.pdf', }); - it('does not save file:// URLs in document metadata', function () { - const { fakePDFViewerApplication, pdfMetadata } = createPDFMetadata({ - fingerprint: 'fakeFingerprint', - url: 'file://fake.pdf', - }); - const expectedMetadata = { - link: [ - { - href: - 'urn:x-pdf:' + fakePDFViewerApplication.pdfDocument.fingerprint, - }, - ], - }; - - return pdfMetadata.getMetadata().then(function (actualMetadata) { - assert.equal(actualMetadata.link.length, 1); - assert.equal( - actualMetadata.link[0].href, - expectedMetadata.link[0].href - ); - }); + const metadata = await pdfMetadata.getMetadata(); + + const fileLink = metadata.link.find(link => + link.href.includes('file://') + ); + assert.isUndefined(fileLink); + }); + + it('gets the title from the dc:title field', async () => { + const { pdfMetadata } = createPDFMetadata(); + const metadata = await pdfMetadata.getMetadata(); + assert.equal(metadata.title, testMetadata.metadata['dc:title']); + }); + + it('gets the title from the documentInfo.Title field', async () => { + const { pdfMetadata } = createPDFMetadata({ + title: 'Some title', + url: 'http://fake.com/', }); + + const metadata = await pdfMetadata.getMetadata(); + + assert.equal(metadata.title, 'Some title'); }); }); }); From ab997a8ac7983f0e6d8b01d5d1782a10afe64e6b Mon Sep 17 00:00:00 2001 From: Robert Knight Date: Wed, 5 May 2021 14:55:23 +0100 Subject: [PATCH 3/4] Convert remaining promise chains to async/await Convert remaining promise chains in `PDFMetadata` tests to async/await. --- .../integrations/test/pdf-metadata-test.js | 39 +++++++++---------- 1 file changed, 18 insertions(+), 21 deletions(-) diff --git a/src/annotator/integrations/test/pdf-metadata-test.js b/src/annotator/integrations/test/pdf-metadata-test.js index 1fecdffe9e5..e38029fc460 100644 --- a/src/annotator/integrations/test/pdf-metadata-test.js +++ b/src/annotator/integrations/test/pdf-metadata-test.js @@ -192,7 +192,7 @@ describe('PDFMetadata', function () { // The `initializedPromise` param simulates different versions of PDF.js with // and without the `PDFViewerApplication.initializedPromise` API. [true, false].forEach(initializedPromise => { - it('does not wait for the PDF to load if it has already loaded', function () { + it('does not wait for the PDF to load if it has already loaded', async () => { const fakePDFViewerApplication = new FakePDFViewerApplication('', { initializedPromise, }); @@ -202,9 +202,8 @@ describe('PDFMetadata', function () { fingerprint: 'fakeFingerprint', }); const pdfMetadata = new PDFMetadata(fakePDFViewerApplication); - return pdfMetadata.getUri().then(function (uri) { - assert.equal(uri, 'http://fake.com/'); - }); + const uri = await pdfMetadata.getUri(); + assert.equal(uri, 'http://fake.com/'); }); }); @@ -227,37 +226,35 @@ describe('PDFMetadata', function () { }; } - describe('#getUri', function () { - it('returns the non-file URI', function () { + describe('#getUri', () => { + it('returns the non-file URI', async () => { const { pdfMetadata } = createPDFMetadata(); - return pdfMetadata.getUri().then(function (uri) { - assert.equal(uri, 'http://fake.com/'); - }); + const uri = await pdfMetadata.getUri(); + assert.equal(uri, 'http://fake.com/'); }); - it('returns the fingerprint as a URN when the PDF URL is a local file', function () { + it('returns the fingerprint as a URN when the PDF URL is a local file', async () => { const { pdfMetadata } = createPDFMetadata({ url: 'file:///test.pdf', fingerprint: 'fakeFingerprint', }); - return pdfMetadata.getUri().then(function (uri) { - assert.equal(uri, 'urn:x-pdf:fakeFingerprint'); - }); + const uri = await pdfMetadata.getUri(); + assert.equal(uri, 'urn:x-pdf:fakeFingerprint'); }); - it('resolves relative URLs', () => { + it('resolves relative URLs', async () => { const { fakePDFViewerApplication, pdfMetadata } = createPDFMetadata({ url: 'index.php?action=download&file_id=wibble', fingerprint: 'fakeFingerprint', }); - return pdfMetadata.getUri().then(uri => { - const expected = new URL( - fakePDFViewerApplication.url, - document.location.href - ).toString(); - assert.equal(uri, expected); - }); + const uri = await pdfMetadata.getUri(); + + const expected = new URL( + fakePDFViewerApplication.url, + document.location.href + ).toString(); + assert.equal(uri, expected); }); }); From b06e863204f271c5b1eaecc6eb131941883ee6ee Mon Sep 17 00:00:00 2001 From: Robert Knight Date: Tue, 11 May 2021 09:40:05 +0100 Subject: [PATCH 4/4] Improve some test descriptions and add a comment - Clarify what a "non-file URL" means and add an additional test case for HTTPS URLs - Add a comment to clarify the precedence order of titles. The tests could more directly check that the expected precedence order is respected, but the logic here is expected to change very soon, so just add a comment for now. --- .../integrations/test/pdf-metadata-test.js | 19 ++++++++++++++----- 1 file changed, 14 insertions(+), 5 deletions(-) diff --git a/src/annotator/integrations/test/pdf-metadata-test.js b/src/annotator/integrations/test/pdf-metadata-test.js index e38029fc460..fa63905b3b4 100644 --- a/src/annotator/integrations/test/pdf-metadata-test.js +++ b/src/annotator/integrations/test/pdf-metadata-test.js @@ -227,13 +227,15 @@ describe('PDFMetadata', function () { } describe('#getUri', () => { - it('returns the non-file URI', async () => { - const { pdfMetadata } = createPDFMetadata(); - const uri = await pdfMetadata.getUri(); - assert.equal(uri, 'http://fake.com/'); + ['http://fake.com/', 'https://example.com/test.pdf'].forEach(pdfURL => { + it('returns the PDF URL if it is an HTTP(S) URL', async () => { + const { pdfMetadata } = createPDFMetadata({ url: pdfURL }); + const uri = await pdfMetadata.getUri(); + assert.equal(uri, pdfURL); + }); }); - it('returns the fingerprint as a URN when the PDF URL is a local file', async () => { + it('returns the fingerprint as a URN when the PDF URL is a file:// URL', async () => { const { pdfMetadata } = createPDFMetadata({ url: 'file:///test.pdf', fingerprint: 'fakeFingerprint', @@ -295,6 +297,13 @@ describe('PDFMetadata', function () { assert.isUndefined(fileLink); }); + // In order, the title is obtained from: + // 1. The `dc:title` field + // 2. The `documentInfo.Title` field + // 3. The `title` property of the HTML `document` (which PDF.js in turn + // initializes based on the filename from the `Content-Disposition` header + // or URL if that is not available) + it('gets the title from the dc:title field', async () => { const { pdfMetadata } = createPDFMetadata(); const metadata = await pdfMetadata.getMetadata();