Test that all the builders produce the same results.#366
Test that all the builders produce the same results.#366xStrom merged 3 commits intolinebender:mainfrom
Conversation
nicoburns
left a comment
nicoburns
left a comment
There was a problem hiding this comment.
Generally seems good, but I'd like to make sure we're aligned on our understanding of how LayoutContexts work (I don't understand why you're using multiple in these tests).
parley/src/tests/test_builders.rs
Outdated
| /// LayoutContext D - Tree for dirt | ||
| /// LayoutContext D - Ranged from dirty | ||
| /// ``` | ||
| fn compute( |
There was a problem hiding this comment.
I think this ought to be called something more specific. assert_builders_produce_same_result or something?
There was a problem hiding this comment.
(Or just builders_produce_same_result)
| let mut lcx_a: LayoutContext<ColorBrush> = LayoutContext::new(); | ||
| let mut lcx_b: LayoutContext<ColorBrush> = LayoutContext::new(); | ||
| let mut lcx_c: LayoutContext<ColorBrush> = LayoutContext::new(); | ||
| let mut lcx_d: LayoutContext<ColorBrush> = LayoutContext::new(); |
There was a problem hiding this comment.
Perhaps you want to test this separately, but my understanding is that you should be able to reuse the same LayoutContext for all builders without affecting results.
There was a problem hiding this comment.
(unless my mental model is wrong, I would consider violations of that a bug)
There was a problem hiding this comment.
Your understanding is correct and this test converts that "should" part into a "must". The test won't pass unless it's actually true. It tests various ways that a LayoutContext could fail to meet that goal.
Violations of this would indeed be a bug. A bug that this test is designed to reveal.
parley/src/tests/test_builders.rs
Outdated
| ); | ||
|
|
||
| // Testing idempotence of ranged builder creation | ||
| let mut lcx_a_rb_two = lcx_a.ranged_builder(&mut fcx, text, scale, quantize); |
There was a problem hiding this comment.
Think this would read a lot clearer if only the actual LayoutContexts had lcx in the name.
There was a problem hiding this comment.
Alright, I refactored the function so that the layout creation happens in a different function. This allowed me to remove these confusing variable names.
3 participants
This will help ensure that the builders won't diverge in behavior as we continue modifying them. It also tests the reusability of
LayoutContext.There are known cases where the builders do currently diverge. For example:
Would be good to have it catch the builders disagreeing on default font size or missing scale multipliers. While adding this test is trivial within the framework of this PR, unfortunately the solution isn't super trivial. So I will add the default style test along with the code fix in a separate PR.
For example see Add a test for nesting spans with the tree builder. #365.
However, this gets into questions about supporting things like margin/padding on spans and how exactly the common core span API should look like.
I wanted to keep this PR here limited to the cases that don't require any builder modification. So that we can at least start protecting the partial commonality that the builders have. This will also mean that the follow-up PRs that do change builder code won't be burdened by the noise of adding a bunch of this testing framework code.