Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
sffc opened this issue Jul 10, 2024 · 2 comments
Labels
C-test-infra Component: Integration test infrastructure discuss Discuss at a future ICU4X-SC meeting

Comments

@sffc
Copy link
Member

sffc commented Jul 10, 2024

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 (#5200 and #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?

@sffc sffc added C-test-infra Component: Integration test infrastructure discuss Discuss at a future ICU4X-SC meeting labels Jul 10, 2024
@robertbastian
Copy link
Member

We have broken this invariant multiple times in the last few weeks (#5200 and #5039).

We have broken this invariant (semver) three times ever: tinystr pre-1.0 (#2428), zerovec in 1.3 (#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
Copy link
Member Author

sffc commented Jul 11, 2024

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. #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.

@sffc sffc added this to the Priority Backlog ⟨P3⟩ milestone Jul 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-test-infra Component: Integration test infrastructure discuss Discuss at a future ICU4X-SC meeting
Projects
None yet
Development

No branches or pull requests

2 participants