-
Notifications
You must be signed in to change notification settings - Fork 9
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Increase test coverage #91
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is great, thank-you! I'm going to merge this, although I had a minor query about the last adjustment ratio value for each of the two test cases.
); | ||
assert.deepEqual( | ||
adjRatios, | ||
[0.857, 0.0, 0.28, 1.0, 0.067, -0.278, 0.536, -0.167, 0.7, -0.176, 0.357, 0.049], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not certain it is guaranteed that these float literals will be converted to the exact same float values as the result of parsing the truncated number, in all environments. If a problem is encountered, assert.closeTo
could be used.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought about using closeTo
, but there wasn't a deepCloseTo
like there is a deepEqual
so I would have had to iterate over the list. Happy to switch to that approach if you think it is preferable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm happy to leave this as-is. My concern is only theoretical at the moment.
); | ||
assert.deepEqual( | ||
adjRatios, | ||
[0.857, 0.0, 0.28, 1.0, 0.067, -0.278, 0.536, -0.167, 0.7, -0.176, 0.357, 0.049], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I notice the book image shows 0.000
as the final adjustment ratio rather than 0.049
. Do you know the reason for that? Does it matter at all?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is because you define MAX_COST
to be 1000, but Knuth & Plass's calculations are based on a MAX_COST
of 100,000. I chose to be consistent with the rest of the code in this repository, but if you want to be truly faithful to the book I can submit this PR with passing tests:
diff --git a/test/layout-test.ts b/test/layout-test.ts
index 2a0f21d..1e0d274 100644
--- a/test/layout-test.ts
+++ b/test/layout-test.ts
@@ -232,9 +232,9 @@ function frogPrinceItems(): TextInputItem[] {
}
};
const epilogue: TextInputItem[] = [
- { type: 'penalty', width: 0, cost: 1000, flagged: false },
- { type: 'glue', width: 0, stretch: 1000, shrink: 0, text: '' } as TextGlue,
- { type: 'penalty', width: 0, cost: -1000, flagged: true },
+ { type: 'penalty', width: 0, cost: 100000, flagged: false },
+ { type: 'glue', width: 0, stretch: 100000, shrink: 0, text: '' } as TextGlue,
+ { type: 'penalty', width: 0, cost: -100000, flagged: true },
];
return frogPrinceItemsImpl(frogPrinceText, prologue, betweenWords, epilogue);
}
@@ -277,7 +277,7 @@ describe('layout', () => {
);
assert.deepEqual(
adjRatios,
- [0.857, 0.0, 0.28, 1.0, 0.067, -0.278, 0.536, -0.167, 0.7, -0.176, 0.357, 0.049],
+ [0.857, 0.0, 0.28, 1.0, 0.067, -0.278, 0.536, -0.167, 0.7, -0.176, 0.357, 0.0],
);
});
@@ -305,7 +305,7 @@ describe('layout', () => {
);
assert.deepEqual(
adjRatios,
- [0.774, 0.179, 0.629, 0.545, 0.0, 0.079, 0.282, 0.294, 0.575, 0.353],
+ [0.774, 0.179, 0.629, 0.545, 0.0, 0.079, 0.282, 0.294, 0.575, 0.004],
);
});
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is because you define MAX_COST to be 1000, but Knuth & Plass's calculations are based on a MAX_COST of 100,000.
Ah, thanks for the clarification. That's fine to leave as-is for the moment then.
); | ||
assert.deepEqual( | ||
adjRatios, | ||
[0.774, 0.179, 0.629, 0.545, 0.0, 0.079, 0.282, 0.294, 0.575, 0.353], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The last adjustment ratio value also differs from the book here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto
The PDF at http://www.eprg.org/G53DOC/pdfs/knuth-plass-breaking.pdf is missing some information that is in the printed version of Digital Typography; namely, the widths of the characters in the examples. This information allows us to write tests to verify that the behavior is exactly as specified in the Knuth & Plass paper, all the way down to the adjustment ratios being identical to the examples given in the paper. The relevant pages from the printed book are enclosed.
CC @robertknight