RX 7900 Fixes and refactoring for error reporting #279
Conversation
|
This is how errors look with this PR (last patch reverted for demonstration purposes)
|
There was a problem hiding this comment.
Overall, I really like these changes, it should make tracking down issues in bug reports notably easier.
It looks like there's a test that needs to be updated: https://github.com/ilya-zlobintsev/LACT/actions/runs/8131682866/job/22222045972?pr=279
In-line
force-pushed
the
rx7900-xtx-fixes
branch
from
d438e9a to
9401074
Compare
lact-schema/src/tests.rs
Outdated
| let expected_response = json!({ | ||
| "status": "error", | ||
| "data": "my super error" | ||
| }); | ||
| let response = Response::<()>::from(anyhow!("my super error")); | ||
|
|
||
| let response = Response::<()>::Error("my super error".to_owned()); | ||
| let json_value = serde_json::to_value(&response).unwrap(); | ||
| let parsed_response: Response<()> = serde_json::from_value(json_value).unwrap(); | ||
|
|
||
| assert_eq!(serde_json::to_value(response).unwrap(), expected_response); | ||
| match (response, parsed_response) { | ||
| (Response::Error(first), Response::Error(second)) => { | ||
| assert_eq!(second.to_string(), first.to_string()) | ||
| } | ||
| _ => unreachable!(), | ||
| }; |
There was a problem hiding this comment.
The goal of these tests is to check the actual json representation of responses, not just that serialization is working properly. So it should still have an expected_response json literal that's compared against the serialized form.
I think here it would be especially useful to check the json representation of an error with a backtrace. If i'm understanding it right, it should be a nested error object.
There was a problem hiding this comment.
@ilya-zlobintsev actual JSON representation is implementation defined in the serde-error crate. I think it doesn't matter what actual format is uses. You would need to update test after every serde-error dependency update.
There was a problem hiding this comment.
What matters is that it produces same output after serialization/de-serialization.
There was a problem hiding this comment.
I'm not sure how to implement that test properly without hard pinning the dependency
There was a problem hiding this comment.
@ilya-zlobintsev do you have any suggestions?
There was a problem hiding this comment.
While the main purpose of the JSON API is communication between the GUI and the daemon, it's not an arbitrary internal API as it's possible to make third party scripts that interact with it. For this reason, I would like to avoid changing the JSON representation of requests/responses when possible. Of course this PR changes the error format, which is fine, but it would be best to avoid accidentally changing it again in the future.
In practice I don't think it's going to be an issue, serde-error didn't have any functional changes in 4 years, but I would still like to have a test that defines what the error representation is.
There was a problem hiding this comment.
@ilya-zlobintsev Okay, I pinned the dependency and updated test. I understand your point. Thanks for patience!
|
The fedora package build error seems to be related to a missing signing key, you can ignore it. |
In-line
force-pushed
the
rx7900-xtx-fixes
branch
from
9401074 to
ed20ca6
Compare
Hi, Dear @ilya-zlobintsev thank you for amazing peace of open source software!
A few key points about this PR.