Add a unit-test to check that ProblematicCharRanges contains valid entries
#7540
Conversation
|
/botio unittest |
From: Bot.io (Windows)ReceivedCommand cmd_unittest from @Snuffleupagus received. Current queue size: 0 Live output at: http://107.22.172.223:8877/db3fc490f8aee16/output.txt |
From: Bot.io (Linux)ReceivedCommand cmd_unittest from @Snuffleupagus received. Current queue size: 0 Live output at: http://107.21.233.14:8877/0ce4c90407ed3b0/output.txt |
From: Bot.io (Windows)SuccessFull output at http://107.22.172.223:8877/db3fc490f8aee16/output.txt Total script time: 2.09 mins
|
From: Bot.io (Linux)FailedFull output at http://107.21.233.14:8877/0ce4c90407ed3b0/output.txt Total script time: 2.11 mins
|
|
That test failure shouldn't be the fault of this PR, trying again... /botio unittest |
From: Bot.io (Windows)ReceivedCommand cmd_unittest from @Snuffleupagus received. Current queue size: 0 Live output at: http://107.22.172.223:8877/c3ea71317998d82/output.txt |
From: Bot.io (Linux)ReceivedCommand cmd_unittest from @Snuffleupagus received. Current queue size: 0 Live output at: http://107.21.233.14:8877/600ffc521137e12/output.txt |
From: Bot.io (Linux)SuccessFull output at http://107.21.233.14:8877/600ffc521137e12/output.txt Total script time: 1.72 mins
|
From: Bot.io (Windows)SuccessFull output at http://107.22.172.223:8877/c3ea71317998d82/output.txt Total script time: 1.88 mins
|
…entries When adding new entries to `ProblematicCharRanges`, you have to be careful to not make any mistakes since that could cause glyph mapping issues. Currently the existing reference tests should probably help catch any errors, but based on experience I think that having a unit-test which specifically checks `ProblematicCharRanges` would be both helpful and timesaving when modifying/reviewing changes to this code. Hence this patch which adds a function (and unit-test) that is used to validate the entries in `ProblematicCharRanges`, and also checks that we don't accidentally add more character ranges than the Private Use Area can actually contain. The way that the validation code, and thus the unit-test, is implemented also means that we have an easy way to tell how much of the Private Use Area is potentially utilized by re-mapped characters.
|
/botio unittest |
From: Bot.io (Windows)ReceivedCommand cmd_unittest from @Snuffleupagus received. Current queue size: 0 Live output at: http://107.22.172.223:8877/6b3e3527c9fd22e/output.txt |
From: Bot.io (Linux)ReceivedCommand cmd_unittest from @Snuffleupagus received. Current queue size: 0 Live output at: http://107.21.233.14:8877/9ce3782ea35d981/output.txt |
From: Bot.io (Linux)SuccessFull output at http://107.21.233.14:8877/9ce3782ea35d981/output.txt Total script time: 1.69 mins
|
From: Bot.io (Windows)SuccessFull output at http://107.22.172.223:8877/6b3e3527c9fd22e/output.txt Total script time: 2.04 mins
|
|
@timvandermeij Is this something that you'd feel comfortable reviewing? |
|
I'm a bit confused about why this has to live in |
|
|
||
| describe('Fonts', function() { | ||
| it('checkProblematicCharRanges', function() { | ||
| var EXPECTED_PERCENTAGE = 45; |
There was a problem hiding this comment.
Where does this value come from?
There was a problem hiding this comment.
It comes from the fact that we currently utilize approximately 44% of the Private Use Area (PUA).
Currently we have no way of knowing how much of the PUA that the remapped characters can potentially make use of, and by having this test fail when you add a number of new char ranges it becomes a lot clearer how the PUA utilization increases over time.
We obviously want to ensure, first of all, that we don't try to add more char ranges than the PUA can actually hold.
When you add new char ranges, you might have to increase this constant, which I feel should help both the developer and reviewer to notice that we're now using more of the PUA than before.
So by having this unit-test that will fail if you add a bunch of new char ranges, we will get advance warning if in the future we're about to run out of space in the PUA.
A bit long-winded perhaps, but I hope it makes sense! It not, just ask :-)
|
Aside from the comments above, this looks really good. I recently merged one of your other PRs that touched |
First of all, I prefer having all of the validation consolidated in just one simple function that the unit-test calls, since it reduces clutter in the test file.
Agreed, my intent with this patch was to simplify the life of both developers and reviewers :-) |
|
/botio unittest |
From: Bot.io (Windows)ReceivedCommand cmd_unittest from @timvandermeij received. Current queue size: 0 Live output at: http://107.22.172.223:8877/c845fa23fbb32be/output.txt |
From: Bot.io (Linux)ReceivedCommand cmd_unittest from @timvandermeij received. Current queue size: 0 Live output at: http://107.21.233.14:8877/2b1a0cc5813dcd6/output.txt |
From: Bot.io (Linux)SuccessFull output at http://107.21.233.14:8877/2b1a0cc5813dcd6/output.txt Total script time: 1.71 mins
|
From: Bot.io (Windows)SuccessFull output at http://107.22.172.223:8877/c845fa23fbb32be/output.txt Total script time: 1.96 mins
|
|
Thank you! |
When adding new entries to
ProblematicCharRanges, you have to be careful to not make any mistakes since that could cause glyph mapping issues.Currently the existing reference tests should probably help catch any errors, but based on experience I think that having a unit-test which specifically checks
ProblematicCharRangeswould be both helpful and timesaving when modifying/reviewing changes to this code.Hence this patch which adds a function (and unit-test) that is used to validate the entries in
ProblematicCharRanges, and also checks that we don't accidentally add more character ranges than the Private Use Area can actually contain.The way that the validation code, and thus the unit-test, is implemented also means that we have an easy way to tell how much of the Private Use Area is potentially utilized by re-mapped characters.
Note: I suppose that the test function could also have been placed inside an IIFE that is only invoked in
!PRODUCTIONmode, but having a unit-test seemed like a good approach to me.