Split the font_spec.js unit-tests into cff_parser_spec.js and type1_parser_spec.js

[Snuffleupagus]

Re: issue #7261.

Given the we have gulp fonttest, which tests the fonts.js functionality at a higher level, and that we have a lot of font specific reference tests, I'm not convinced that we also need unit-tests for it.

Note: I'm setting the [WIP] label, pending agreement on the above.

[Snuffleupagus]

/botio unittest

[pdfjsbot]

From: Bot.io (Windows)

---

Received

Command cmd_unittest from @Snuffleupagus received. Current queue size: 0

Live output at: http://107.22.172.223:8877/2f97b4a3230248c/output.txt

[pdfjsbot]

From: Bot.io (Linux)

---

Received

Command cmd_unittest from @Snuffleupagus received. Current queue size: 0

Live output at: http://107.21.233.14:8877/7a1726ae5436106/output.txt

[pdfjsbot]

From: Bot.io (Linux)

---

Success

Full output at http://107.21.233.14:8877/7a1726ae5436106/output.txt

Total script time: 1.65 mins

• Unit Tests: Passed

[pdfjsbot]

From: Bot.io (Windows)

---

Success

Full output at http://107.22.172.223:8877/2f97b4a3230248c/output.txt

Total script time: 1.98 mins

• Unit Tests: Passed

[timvandermeij]

Sounds reasonable to me. @yurydelendik, what do you think?

[brendandahl]

Given the we have gulp fonttest, which tests the fonts.js functionality at a higher level, and that we have a lot of font specific reference tests, I'm not convinced that we also need unit-tests for it.

I highly prefer unit test over the higher level tests. Debugging the higher level ref tests when broken is often very painful, where as a unit test the issue is usually immediately obvious. In hindsight, I think we should have made much more of our ref tests into font unit tests. You've probably also run into the situation where a reftest breaks and you have no idea why without some pretty deep digging.

[timvandermeij]

You're right. Let's keep on tracking the addition of more unit tests in #7261, but I think this patch is nevertheless good to have because the two parsers are also in separate files in src/core.

[Snuffleupagus]

Debugging the higher level ref tests when broken is often very painful, where as a unit test the issue is usually immediately obvious. In hindsight, I think we should have made much more of our ref tests into font unit tests.

That is certainly a very valid point; one that I didn't really consider!
One possible problem with creating small/self-contained unit-tests, is that for e.g. TrueType fonts (which is what most of my font patches have touched), it might not be that easy to do.
For example: Most of the helper functions in Font_checkAndRepair are local, and you'd probably want a way to test only parts of that functionality in an issue/bug specific unit-test. Should we perhaps split some of those local functions out into "class" methods instead?

You've probably also run into the situation where a reftest breaks and you have no idea why without some pretty deep digging.

I have, on more than one occasion, and I agree that it's no fun at all.

[...] but I think this patch is nevertheless good to have because the two parsers are also in separate files in src/core.

Yes, as Tim points out, this patch is more of a bookkeeping one, moving existing unit-tests to the correct files. Please note that it does not remove any existing tests.

@brendandahl So in summary: If we block this PR on adding unit-tests for fonts.js, I'm not sure when it'll land, so can we take this and track adding unit-tests for fonts.js in e.g. #7261 instead?

[brendandahl]

Should we perhaps split some of those local functions out into "class" methods instead?

I'm not sure it's worth retroactively adding unit tests for these, for new bugs though unit tests would be preferable.

If we block this PR

No need to block.