Refactor PDFMetadata tests
#3373
Conversation
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.
Codecov Report
@@ Coverage Diff @@
## master #3373 +/- ##
=======================================
Coverage 98.44% 98.44%
=======================================
Files 213 213
Lines 7761 7761
Branches 1752 1752
=======================================
Hits 7640 7640
Misses 121 121 Continue to review full report at Codecov.
|
- 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.
Convert remaining promise chains in `PDFMetadata` tests to async/await.
robertknight
force-pushed
the
refactor-pdf-metadata-tests
branch
from
7f7f722 to
ab997a8
Compare
There was a problem hiding this comment.
The conversion to async/await and other modernizing and structure updates make sense just fine.
I'm not going to pretend to fully have my head wrapped around the multiple levels of fakes—let me know if you'd like me to invest time in that deeper understanding, or if this surface-level evaluation of the tests themselves suffices!
| } | ||
|
|
||
| describe('#getUri', () => { | ||
| it('returns the non-file URI', async () => { |
There was a problem hiding this comment.
What exactly does non-file mean here?
There was a problem hiding this comment.
It means any URL with a scheme other than file:. In practice it means a URL with a scheme that is http: or https:. I'll update the test description to clarify.
| assert.equal(metadata.title, testMetadata.metadata['dc:title']); | ||
| }); | ||
|
|
||
| it('gets the title from the documentInfo.Title field', async () => { |
There was a problem hiding this comment.
It might be helpful to detail what context causes the title to come from documentInfo.title vs. dc:title?
There was a problem hiding this comment.
I've added a comment above these tests to spell out the intended behavior.
- 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.
This PR is some refactoring of the tests for the code that reads metadata (URL, fingerprint, title) from PDFs in preparation for fixing #3372. There are no functional changes.
See individual commit messages for full details, but in summary:
beforeEachblocks to helper functions so that it can be customized by individual testsgetMetadataso that individual tests only check properties of the result they are interested in, rather than the whole result.linksproperty of thegetMetadataresult