[BUG] tests fail with unicode-width 0.1.12 but not with 0.1.13 (should we support both in tests too?)

[correabuscar]

Describe the bug

tl;dr: some tests still fail for users that had old git clone and didn't cargo update after git pull, even though the lib works as intended.
But since the lib can handle control chars of either width 0 or 1, should I mod the currently failing tests to handle both width 0 or 1 ? and should I do it without introducing a build.rs ? or just do nothing and thus allow tests to catch when width is != 1 in the future ?

---

In this commit:
af7c3d7

I see the intention was as you've mentioned:

I cherry-picked the last commit (just removed the \0 width test, so we're not tied to unicode-width 1.1.13)

in other words, you wanted it to work with either 0.1.12 or 0.1.13 (but also for tests to not fail as well), as you've mentioned here also:

The crash should now be fixed with the latest commit, which ignores \0 (when using unicode-width < 0.1.13), or replaces it with � (when using unicode-width >= 0.1.13).

however, with unicode-width 0.1.12, all control chars have width 0 (I've just tested) therefore this whole test(which already covers \0 as well) will fail:

cursive/cursive-core/src/utils/lines/spans/tests.rs

Lines 32 to 61 in 332a6a4

	fn test_control_chars_have_width_1() {
	use unicode_width::UnicodeWidthStr;
	let control_chars = [
	"\u{0000}", "\u{0001}", "\u{0002}", "\u{0003}", "\u{0004}", "\u{0005}", "\u{0006}",
	"\u{0007}", "\u{0008}", "\u{0009}", "\u{000A}", "\u{000B}", "\u{000C}", "\u{000D}",
	"\u{000E}", "\u{000F}", "\u{0010}", "\u{0011}", "\u{0012}", "\u{0013}", "\u{0014}",
	"\u{0015}", "\u{0016}", "\u{0017}", "\u{0018}", "\u{0019}", "\u{001A}", "\u{001B}",
	"\u{001C}", "\u{001D}", "\u{001E}", "\u{001F}", "\u{007F}", "\u{0080}", "\u{0081}",
	"\u{0082}", "\u{0083}", "\u{0084}", "\u{0085}", "\u{0086}", "\u{0087}", "\u{0088}",
	"\u{0089}", "\u{008A}", "\u{008B}", "\u{008C}", "\u{008D}", "\u{008E}", "\u{008F}",
	"\u{0090}", "\u{0091}", "\u{0092}", "\u{0093}", "\u{0094}", "\u{0095}", "\u{0096}",
	"\u{0097}", "\u{0098}", "\u{0099}", "\u{009A}", "\u{009B}", "\u{009C}", "\u{009D}",
	"\u{009E}", "\u{009F}",
	];
	for c in &control_chars {
	let width = c.width();
	assert_eq!(
	c.chars().count(),
	1,
	"it's supposed to be a string of 1 char"
	);
	let unicode_escape = format!("\\u{{{:04X}}}", c.chars().last().unwrap() as u32);
	assert_eq!(
	width, 1,
	"Width of control character {} is not 1",
	unicode_escape
	);
	}
	}

so not just the test for \0 that got removed in the aforementioned commit.

I'm not sure(unless add build.rs¹) how to conditionally make the test depend on 0.1.12 and check for width 0, and also if >=0.1.13 check for width 1, but the initial intent was that if the assumed width changes in a future unicode-width update, we should have tests fail, else we wouldn't know that bugs creep in due to width being different now (such as: sizes and shadows being off), even though the lib can current handle both widths: 0 or 1. Maybe given this, can mod the test for width being < 2 but there's the test below which might need some tweaking too(if build.rs introduction isn't wanted).

Alternatively, maybe it doesn't matter in this case that the cargo test will fail for users which just git pull-ed new changes(due to them having their own Cargo.lock locally, because it's in .gitignore), because, while this test fails, the lib does work as expected, and the user is in fact expected to cargo update anyway.

Additionally this test will fail too, for those users that didn't cargo update to get unicode-width 0.1.13: $ cargo test --example select_test -- --nocapture, in particular tests::control_chars_become_replacement_char and tests::nuls_become_replacement_char.

But again, all the mentioned tests only fail for users that didn't cargo update, not for anyone who did a fresh git clone for example. And we kinda want these tests to fail to catch future width changes, I'd think.

To Reproduce

1. git clone
2. force use unicode-width 0.1.12

diff --git a/cursive/Cargo.toml b/cursive/Cargo.toml
index 3a26f0b..810edc0 100644
--- a/cursive/Cargo.toml
+++ b/cursive/Cargo.toml
@@ -19,7 +19,7 @@ cursive_core = { path = "../cursive-core", version= "0.3.0"}
 crossbeam-channel = "0.5"
 cfg-if = "1"
 unicode-segmentation = "1"
-unicode-width = "0.1"
+unicode-width = "=0.1.12"
 lazy_static = "1"
 libc = "0.2"
 maplit = { version = "1.0", optional = true }

3. cargo update
4. cargo test test_control_chars_have_width_1 -- --nocapture
5. cargo test --example select_test -- --nocapture

Expected behavior
for users with stale Cargo.lock, tests wouldn't fail.

Screenshots
N/A

Environment

• Operating system used
• Backend used: ncurses (the default one)
• Current locale (run locale in a terminal)

LANG=en_US.utf8
LC_CTYPE="en_US.utf8"
LC_NUMERIC="en_US.utf8"
LC_TIME="en_US.utf8"
LC_COLLATE="en_US.utf8"
LC_MONETARY="en_US.utf8"
LC_MESSAGES="en_US.utf8"
LC_PAPER="en_US.utf8"
LC_NAME="en_US.utf8"
LC_ADDRESS="en_US.utf8"
LC_TELEPHONE="en_US.utf8"
LC_MEASUREMENT="en_US.utf8"
LC_IDENTIFICATION="en_US.utf8"
LC_ALL=

• Cursive version (from crates.io, from git, ...)
  latest commit which currently is: 332a6a4

Additional context
nits: I forgot to mention that in commit in code comments, it's unicode-width instead of unicode-segmentation(which is at 1.11.0 and has no effect on width, from what I can tell)

Footnotes

1. without introducing a build.rs(which would set an expected width(based on unicode-width version, so it would be either 0 or 1) in a new .rs that we'd include!() and thus can use that in the test). I'd be happy to do it, if you'd just let me know that you want this. ↩

[correabuscar]

I'll make a proof-of-concept PR that shows how it would look like if the tests supported both widths (0 and 1) because the lib already does, and without needing a build.rs; it doesn't have to be merged, but it can be if this version of handling the issue is chosen.