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

fix(tests): revert test-breaking changes of e5c7ae4 #339

Merged
merged 2 commits into from
Feb 7, 2024

Conversation

TheLostLambda
Copy link
Contributor

The commit e5c7ae4 seemed to (potentially erronously?) remove a number of spaces following the left | bar of some graphical report handler tests. That change had no effect on the syntax_highlighter_on_real_file() test, presumably because the trailing whitespace is removed by strip_ansi_escapes::strip_str(), but it did break the triple_adjacent_highlight() and non_adjacent_hightlight() tests. Restoring the spaces removed in e5c7ae4 fixes the failing tests on main.

The commit e5c7ae4 seemed to (potentially erronously?) remove a number of
spaces following the left `|` bar of some graphical report handler tests.
That change had no effect on the `syntax_highlighter_on_real_file()`
test, presumably because the trailing whitespace is removed by
`strip_ansi_escapes::strip_str()`, but it did break the
`triple_adjacent_highlight()` and `non_adjacent_hightlight()` tests.
Restoring the spaces removed in e5c7ae4 fixes the failing tests on main.
@TheLostLambda
Copy link
Contributor Author

TheLostLambda commented Feb 7, 2024

Well, I fixed the main issue, but this second half has been quite the dramatic journey... It's a issue introduced by a combination of a new nightly Rust version (used in that CI test to use the unstable minimal-versions flag) and an old version of ahash. I had to upgrade my nightly, use the minimum crate versions, and enable the fancy feature to run into this issue. Seems like ahash has been naughty and was actually blocking a PR on the Rust repo for a while tkaitchuck/aHash#200 (comment) and this issue now is from that pre-patched version of ahash.

It looks like ahash is ultimately getting pulled in by the fancy feature and textwrap — I'll try to bump some versions so that things play nicely with the new nightly release. A lot of stars had to align for that issue to pop up!

@TheLostLambda
Copy link
Contributor Author

Just kidding, the offending ahash version is so deeply nested in textwrap that we have no power over it, even with minimal-versions we're using the newest textwrap.
image

This actually seems to be the issue cautioned against in the unstable feature description for minimal-versions https://doc.rust-lang.org/cargo/reference/unstable.html#minimal-versions — it seems like we really want is the alternative direct-minimal-versions flag, which tests the minimal versions listed in miettes Cargo.tomls, but doesn't touch the dependencies of our dependencies.

I've changed the flag in the CI and bumped a couple of versions that were out of sync between miette and miette-derive and needed to be bumped for things to resolve and compile!

@TheLostLambda
Copy link
Contributor Author

Tada! CI fixed!

@zkat zkat merged commit 6e829f8 into zkat:main Feb 7, 2024
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants