Create CursorTest harness #227

PoignardAzur · 2024-12-11T17:07:55Z

This is a long-awaited feature, letting us write user-friendly tests of Parley's cursor behavior.

tomcur

Really good to see this progress. I think the main concern raised in the Zulip thread is whether terminal output can work well enough for all the errors that may need to be reported.

One possibility is to just debug-print the actual and expected cursors for now.

I think spanning selections should also be in scope for testing, but that can be done later.

PoignardAzur · 2024-12-11T17:17:35Z

One possibility is to just debug-print the actual and expected cursors for now.

The PR already does.

PoignardAzur · 2024-12-11T17:18:02Z

tomcur · 2024-12-11T17:26:36Z

I mean only debug-printing the cursor structs to the terminal: printing a report to the terminal as in the screenshot may not be possible for all the layouts that are interesting to test selection for (RTL text, very long lines, line breaks). A report like that is very useful to have though, perhaps something very similar could be rendered to an image.

parley/src/cursor_test.rs

DJMcNab · 2024-12-11T17:33:48Z

parley/src/cursor_test.rs

+        // TODO - Check that the tested string doesn't include difficult
+        // characters (newlines, tabs, RTL text, etc.)
+        // If it does, we should still print the text on a best effort basis, but
+        // without visual cursors and with a warning that the text may not be accurate.


As discussed on Zulip, the alternative/fallback is to do screenshots. I'd suggest that we should have screenshots unconditionally if this fails, and show the in-terminal version if there is no RTL region (we should strive to handle newlines properly, and we can't possibly detect non-tab extra-wide terminal characters so it's not worth trying)

This doesn't need to happen in this PR, but it might be worth leaving these breakcrumbs in the code

DJMcNab · 2024-12-11T17:37:33Z

parley/src/cursor_test.rs

+    ///
+    /// Uses the same format as assertion failures.
+    #[track_caller]
+    pub fn print_cursor(&self, cursor: Cursor) {


Hmm, this is a bit harder to have with screenshots for debugging. I suppose we can't know if a test will fail until it fails. We could queue these up?

Not a problem for this PR to solve.

DJMcNab · 2024-12-11T17:38:13Z

parley/src/cursor_test.rs

+
+    #[test]
+    fn cursor_next_visual() {
+        let (mut lcx, mut fcx) = (LayoutContext::new(), FontContext::new());


Ideally this would use the build-in Roboto font, but of course it doesn't really matter what font is chosen, so long as it is not fundamentally misbehaving

Actually of course it matters, for "up/down" button pressing test cases

~~Hmm does the font really not matter?~~ Edit: you replied one second before my comment.

I think we shouldn't worry about up-down testing for now, until we have a UI for tests with multi-line strings.

Like, yes, in theory we could just stick to comparing the numeric values of cursors, but if we want to do that we don't need this PR at all, just assert_eq.

DJMcNab · 2024-12-11T17:38:42Z

parley/src/lib.rs

@@ -124,6 +125,7 @@ pub use peniko::Font;

 pub use builder::{RangedBuilder, TreeBuilder};
 pub use context::LayoutContext;
+pub use cursor_test::CursorTest;


What is the advantage of making this public?

For writing tests for something like the text editor widget in Masonry. We can always stop exporting it in future versions if we find nobody uses it.

I can't imagine a user story which would want this exported in its current form, but I can imagine that some of this code would be useful if made more modular.

I do think that the publicly exported "get the debug print of a cursor from the text and its corresponding layout" could be useful (where the layout is used only to find the soft line breaks in the text for debug printing) - caveated of course that it's a best effort. However, if we were actually doing that, we'd probably also want to properly use unicode_width, which I'm not sure we want to do. As it currently stands, this functionality is also tied in with the constructor creating the layout, which is pretty far from ideal.

The "quickly create a cursor from a needle in a string" is also useful to export publicly for people testing e.g. rich text, but again I think it's conflated with the other stuff.

PoignardAzur · 2024-12-12T17:11:05Z

The CI is blowing up. Not sure where these errors are coming from.

PoignardAzur · 2024-12-12T17:15:55Z

To re-iterate what was mentioned in Office Hours:

The purpose of this PR is to be able to write tests for cursor values where the intent of the test can be understood by reading the code of the test alone. This is in contrast to tests where you compare the cursor to a numeric value (the reader needs to visualize what position the number represents) and screenshot tests (the reader needs to open the screenshot file).

In future versions of this API, we'll probably dump screenshots directly to the console where possible, or other similar tricks.

The important part is that in any tests we write, readers should understand the intent of the test form the code alone.

DJMcNab · 2024-12-13T08:58:08Z

The purpose of this PR is to be able to write tests for cursor values where the intent of the test can be understood by reading the code of the test alone.

Again, I agree entirely that we should have this, and I agree that the "needle" based API achieves this very cleverly and unambiguously. But I still don't believe that this infrastructure as provided can possibly be useful for external users, and so we shouldn't export it.
I'd happily approve as-is if this new code were in the test utils module or otherwise cfg(test)ed.

PoignardAzur · 2024-12-13T11:51:37Z

But I still don't believe that this infrastructure as provided can possibly be useful for external users, and so we shouldn't export it.

Do you think there's a downside to exporting it, besides potential confusion?

The obvious downside would be that people might end up depending on it and be upset if we remove it later, but if you think this will never be useful to external users, then that doesn't really apply.

I'd be in favor of exporting it, maybe with a feature flag or a #[doc(hidden)] attribute, and if 6 months later we find out nobody uses it we can make it private again.

DJMcNab · 2024-12-13T13:46:14Z

I strongly agree that some of these features would be useful for someone creating their own editor or similar on top of Parley's primitives. Using a needle to create a cursor is brilliant, nicely simple code but very clear.

However, I'm still really struggling to imagine a user story for someone who would use what you are proposing we export - as this only exposes those features in a very straitjacketed way. I feel that I must be missing something fundamental here.
As far as I can tell, the only possible tests which can be written using this API are for moving a (single) cursor in different ways in entirely plain text - currently on one line, but in the future across many lines. However, the implementations of all the reasonable operations you would want to have (and if we've missed any, it's our bug) live in this crate, and so should be tested here, not by our external users.

My earlier conditional approval still stands. I'm not going to block this if someone else approves.

PoignardAzur · 2024-12-14T09:50:23Z

I've made the implementation private again, and added some TODOs for future reference.

DJMcNab · 2024-12-16T11:10:05Z

parley/src/tests/mod.rs

@@ -1,6 +1,9 @@
 // Copyright 2024 the Parley Authors
 // SPDX-License-Identifier: Apache-2.0 OR MIT

+// TODO - Figure out which tests can run without std


Hmm, why do we want this? Afaik, we only enable no_std when not running tests.

IIRC, the test runner itself requires std so tests are always run with std regardless of whether you enable it anyway.

parley/src/layout/cluster.rs

PoignardAzur added 2 commits December 11, 2024 18:07

Create CursorTest harness

65dc7b4

Add fixme comment

bb41395

tomcur reviewed Dec 11, 2024

View reviewed changes

Fix up print_cursor format

1c9d2c0

DJMcNab reviewed Dec 11, 2024

View reviewed changes

Tweak documentation and values

59f3940

Add advice about testing

8d6f2f5

PoignardAzur added 2 commits December 13, 2024 12:59

Gate on std feature

e57fbaf

Fix clippy lints

e49e3df

Tweak cursor PR

7783ebf

Fix clippy lint

df3322c

DJMcNab approved these changes Dec 16, 2024

View reviewed changes

waywardmonkeys reviewed Dec 16, 2024

View reviewed changes

parley/src/layout/cluster.rs Outdated Show resolved Hide resolved

Address code reviews

e1a3de4

PoignardAzur enabled auto-merge December 22, 2024 15:26

PoignardAzur added this pull request to the merge queue Dec 22, 2024

Merged via the queue into main with commit 65e4c7f Dec 22, 2024
20 checks passed

PoignardAzur deleted the test_api branch December 22, 2024 15:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Create CursorTest harness #227

Create CursorTest harness #227

PoignardAzur commented Dec 11, 2024

tomcur left a comment •

edited

Loading

PoignardAzur commented Dec 11, 2024

PoignardAzur commented Dec 11, 2024

tomcur commented Dec 11, 2024 •

edited

Loading

DJMcNab Dec 11, 2024

DJMcNab Dec 11, 2024

DJMcNab Dec 11, 2024

DJMcNab Dec 11, 2024

tomcur Dec 11, 2024 •

edited

Loading

PoignardAzur Dec 12, 2024

DJMcNab Dec 11, 2024

PoignardAzur Dec 12, 2024

DJMcNab Dec 12, 2024

PoignardAzur commented Dec 12, 2024

PoignardAzur commented Dec 12, 2024

DJMcNab commented Dec 13, 2024

PoignardAzur commented Dec 13, 2024

DJMcNab commented Dec 13, 2024

PoignardAzur commented Dec 14, 2024

DJMcNab Dec 16, 2024

nicoburns Dec 16, 2024

Create CursorTest harness #227

Create CursorTest harness #227

Conversation

PoignardAzur commented Dec 11, 2024

tomcur left a comment • edited Loading

Choose a reason for hiding this comment

PoignardAzur commented Dec 11, 2024

PoignardAzur commented Dec 11, 2024

tomcur commented Dec 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tomcur Dec 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PoignardAzur commented Dec 12, 2024

PoignardAzur commented Dec 12, 2024

DJMcNab commented Dec 13, 2024

PoignardAzur commented Dec 13, 2024

DJMcNab commented Dec 13, 2024

PoignardAzur commented Dec 14, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tomcur left a comment •

edited

Loading

tomcur commented Dec 11, 2024 •

edited

Loading

tomcur Dec 11, 2024 •

edited

Loading