Testing: Edge-case linebreaking matches Chromium#642
Conversation
There was a problem hiding this comment.
I think we need to copy the licenses in parley_dev/assets/fonts/arimo_fonts and parley_dev/assets/fonts/roboto_fonts into this folder if we're going to use these fonts in this way.
There was a problem hiding this comment.
I'm happy enough to oblige, but that's a surprising interpretation to me. Would you be able to expand on why that's needed?
There was a problem hiding this comment.
In particular, I'd like to understand why this same concern didn't apply to the snapshot tests which used these fonts.
There was a problem hiding this comment.
No, I think you're right, because there's no way to recreate the fonts from that CSV data (maybe advance width of glyphs at a stretch). I was initially concerned because the licensed font file is referenced (by name) but I don't think this actually matters.
There was a problem hiding this comment.
For crate names, could we go with these names, for more consistency linebreaking_*, and clarity (on the first name)?
linebreaking_browser_recorderlinebreaking_tests
Open to suggestions.
There was a problem hiding this comment.
I've renamed the recorder and the test. I think that the case generator is at a local maximum; concretely, the name _tests is worse because it doesn't actually contain any tests...
| let word_count = rng.random_range(MIN_WORDS..=MAX_WORDS); | ||
|
|
||
| let mut text = String::new(); | ||
| // TODO: Ideally, this generation would be more free-form, to e.g. test line breaking with punctuation. |
There was a problem hiding this comment.
non-blocking: Do we want to expand beyond just VALID_LETTERS to include numerals and punctuation in this PR? If nothing else, glyphs with narrower advance width (punctuation) might be more likely to trip boundary cases.
There was a problem hiding this comment.
Punctuation is definitely out for now, but the thinking is that we'd expand to that soon after #640 lands. I'll try again with adding numbers though.
There was a problem hiding this comment.
I've added numbers as well. No new failures.
| </head> | ||
|
|
||
| <body> | ||
| <h1>Parley ↔ Chrome line-breaking data collection</h1> |
| //! ```sh | ||
| //! rustup target add wasm32-unknown-unknown | ||
| //! cargo install --locked trunk | ||
| //! trunk serve --open # run from this crate's directory |
There was a problem hiding this comment.
I haven't confirmed this, but Claude suggests --open just uses your default browser, not necessarily Chrome? I don't think there's a way around this, but it's worth documenting. Maybe erroring out after let version = chrome_version(&window); accordingly, but I'm not sure. WDYT?
There was a problem hiding this comment.
Two things:
- I didn't realise we had
--openthere, although I'm not sure I would have changed it if I had seen it - thanks for the callout. That's my bad for not cleaning up that part of Claude's work. - I think erroring if we don't have a Chrome version would be reasonable, although it's worth noting that the way the metadata is provided is my protection against that. That is, a use of this with a non-chromium browser should be caught in review.
|
Interesting, am I understanding correctly that the recorder is intended to be served with Trunk, and then opened manually in Chrome? I wonder if there would be any value in aligning with Taffy's test generator (https://github.com/DioxusLabs/taffy/tree/main/scripts/gentest) which runs Chrome using WebDriver? Perhaps not worth the lift in the short term, but worth considering in future? (Taffy's runner currently uses the |
|
Yeah, that's the current state. There would be some value in having it be automated, but the expectation is that these values change so rarely anyway that it isn't especially valuable. |
3 participants
This new test uses Chromium as a source for line breaking data, and checks that we get the same result in the edge cases.
It does not bring Chrome into our CI; these are instead collected manually and committed in the repo, to ensure stability.
This has requires a few tweaks to how we call Parley to more closely match Chrome, including working around a cache collision bug in Chrome (which will only be reasonably found in a harness like this, so I haven't reported it).
On my machine (which is admittedly quite fast), these tests pass in 3s. As such, I don't expect their performance to be a significant limiting factor here. A release mode run would make them ~instant (0.10s).
AI usage: Code developed with Claude Opus 4.8, and then significantly tweaked to make sane and more understandable. I also used Sonnet to review, which caught a couple of issues.