search

unicode-org / icu4x

Solving i18n for client-side and resource-constrained environments.
https://icu4x.unicode.org
Other
1.32k stars 172 forks source link

Test that old tutorials continue working against pinned ICU4X after `cargo update` #5213

Open sffc opened 1 month ago

sffc commented 1 month ago

I would like us to test that code written against specific ICU4X versions continues working even after cargo update.

We have broken this invariant multiple times in the last few weeks (https://github.com/unicode-org/icu4x/issues/5200 and https://github.com/unicode-org/icu4x/issues/5039). We've done a good job to make sure that people on the 1.x release stream keep working, but anyone who has pinned to a specific 1.3.x or 1.4.x are or were broken for at least some amount of time.

I've heard criticism before that tutorial tests are not semver tests. While this is true, both of the mentioned incompatibilities would show up even without the specific tutorial code. They would show up by simply building a Cargo.toml with icu = "~1.3". The tutorial code just gives us something concrete to run, which I think is beneficial, but if we can't agree on that (I wouldn't be writing this if I didn't think that was a likely outcome), I would be happy even if the test just ran cargo check on some Cargo.toml file without actually running any code.

How should we go about it?

robertbastian commented 1 month ago

We have broken this invariant multiple times in the last few weeks (https://github.com/unicode-org/icu4x/issues/5200 and https://github.com/unicode-org/icu4x/issues/5039).

We have broken this invariant (semver) three times ever: tinystr pre-1.0 (https://github.com/unicode-org/icu4x/issues/2428), zerovec in 1.3 (https://github.com/unicode-org/icu4x/discussions/4093), and fixed_decimal in 1.5 (#5200). All of these could have been detected before they happened.

5039 doesn't have anything to do with cargo update/semver, it was a bug in every single release of our code. We caught it on main (we should've caught it earlier in nightly CI, but that's a separate, fixed, issue), we didn't need to test on older versions to catch it. We have fixed it on main, and backported to all 1.x versions, we don't need to go test that the backports work.

I've heard criticism before that tutorial tests are not semver tests. While this is true, both of the mentioned incompatibilities would show up even without the specific tutorial code. They would show up by simply building a Cargo.toml with icu = "~1.3".

It would have only caught the semver violation. #5039 is not a semver violation and therefore doesn't show up as a compile error. Running a tutorial that hits the bug with older versions on nightly would have caught it (don't know if we have one), but running tests at main on nightly caught it just the same way.

The only bugs your proposal (i.e. building with icu = "~1.3") catches that main CI doesn't catch are semver violations. Obviously not publishing semver violations is preferable to detecting them after publishing, so we should just implement #2570.

Non-compile-time errors caused by cargo update could be detected under your proposal, if we had sufficient test coverage. So instead of focussing on tutorials, we would need to run cargo update + cargo test on old versions. However, if our dependencies strictly follow semver, and we strictly follow semver, these errors do not exist.

sffc commented 1 month ago

We have fixed it on main, and backported to all 1.x versions, we don't need to go test that the backports work. ... It would have only caught the semver violation. https://github.com/unicode-org/icu4x/issues/5039 is not a semver violation and therefore doesn't show up as a compile error.

Not true; 1.3 is broken right now on crates.io. #5039 shows up because upgrading to the latest ~zerovec~ zerovec-derive creates errors like this one against older ICU4X.