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

Add color-eyre to workspace #110

Merged
merged 136 commits into from
Mar 14, 2024
Merged

Add color-eyre to workspace #110

merged 136 commits into from
Mar 14, 2024

Conversation

pksunkara
Copy link
Contributor

@pksunkara pksunkara commented Oct 11, 2023

No description provided.

@ten3roberts
Copy link
Contributor

The test_error_backwards_compatability is failing for cargo test --all --no-default-features.

The test is very complex and has failed in the past when rust std added another function in the backtrace due to some internal change.

I can not reproduce this locally, so it may be because of some rustc version or something else entirely

@ten3roberts
Copy link
Contributor

Ok, I've now found the reason for failure.

It has do to with rustc versions. 1.65 causes it to fail, 1.71 passes

@ten3roberts
Copy link
Contributor

As I suspected, "updating" the "minimal_features" backtrace to match caused other tests using 1.75.0 to fail, since somewhere between those versions, the functions and call stack of the panic machinery changed which causes them to match.

The solution I see here is to ignore the tests for the older version, or don't test backtraces on older compiler versions.

@ten3roberts
Copy link
Contributor

Found a common denominator and a way to make the tests more lenient in the future.

@ten3roberts ten3roberts marked this pull request as ready for review January 25, 2024 13:12
Copy link
Contributor

@thenorili thenorili left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Really good work on the UI tests, those were tough.

I posed a few questions, but the core of it looks ready to me.

Comment on lines +47 to +48
- --no-default-features --features track-caller
- --no-default-features --features auto-install
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How does changing to "--no-default-features" for eyre's track-caller and auto-install tests relate to adding color-eyre to the workspace?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure, I think it was Pavan who added that to increase the test coverage for feature combinations. This is one of the earliest commits

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, it's replacing two feature combinations, i'm not sure that it expands the test coverage. If we want to increase the test coverage we could keep both --features and --no-features.

README.md Show resolved Hide resolved
color-eyre/src/config.rs Outdated Show resolved Hide resolved
color-eyre/tests/data/theme_panic_control_no_spantrace.txt Outdated Show resolved Hide resolved
@thenorili
Copy link
Contributor

Remember not to squash this once it's done!

The test now only considers our part of the backtrace, allowing for
changes in rust std library to not break the test
@thenorili
Copy link
Contributor

Remember not to squash this once it's done!

I'm not sure this will be that simple.

How are we going to preserve history when merging color-eyre into eyre? How did y'all do it for spantraces? Does a rebase work? If so, do we need to ping jane for an exception to the policy?

@pksunkara
Copy link
Contributor Author

Creating a merge commit is enough, isn't it?

@thenorili
Copy link
Contributor

Creating a merge commit is enough, isn't it?

I tested it and yes, that is how that works! Sorry for the alarm : )

@ten3roberts
Copy link
Contributor

This works now (barring the conflict), how do we feel about merging this?

@ten3roberts
Copy link
Contributor

I'll make sure to merge it properly into main to preserve the history

@yaahc yaahc requested a review from thenorili March 14, 2024 21:24
Copy link
Contributor

@thenorili thenorili left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks good! Congrats to both of y'all on landing this : )

@ten3roberts ten3roberts merged commit 7a5c32a into master Mar 14, 2024
29 checks passed
@ten3roberts ten3roberts deleted the color-eyre branch March 14, 2024 21:36
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.