Fix failing test and improve indentation test error message #1135
Conversation
OussamaSaoudi
requested review from
joonyoo181 and
zachschuermann
and removed request for
zachschuermann
| Err(error) => { | ||
| let error_str = error.to_string(); | ||
| assert!( | ||
| error_str.contains(message), |
There was a problem hiding this comment.
was this previously a fuzzy str.contains? Thats the only explanation I could see for why this wasn't caught 🤔
There was a problem hiding this comment.
from my experience, it returned true for exact substring matches (e.g. when i was writing error messages, I had to pass in the exact substring), sooo i don't think it is fuzzy?
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1135 +/- ##
==========================================
- Coverage 83.56% 83.55% -0.02%
==========================================
Files 97 97
Lines 23024 23024
Branches 23024 23024
==========================================
- Hits 19241 19238 -3
- Misses 2793 2794 +1
- Partials 990 992 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| ), | ||
| "Invalid argument error: Incorrect datatype. Expected Struct(metadata Binary, value Binary), got Struct(field_1 Binary, field_2 Binary)", | ||
| // Arrow has different printing for different versions. We use the common prefix | ||
| "Invalid argument error: Incorrect datatype. Expected Struct", |
There was a problem hiding this comment.
should we add a TODO to revert it to the actual error message after we normalize on arrow 55.1+?
There was a problem hiding this comment.
like zach's comment, i think a todo comment would be nice
joonyoo181
left a comment
joonyoo181
left a comment
There was a problem hiding this comment.
I think adding the todo from Zach's comment would be nice. But otherwise, LGTM!
| ), | ||
| "Invalid argument error: Incorrect datatype. Expected Struct(metadata Binary, value Binary), got Struct(field_1 Binary, field_2 Binary)", | ||
| // Arrow has different printing for different versions. We use the common prefix | ||
| "Invalid argument error: Incorrect datatype. Expected Struct", |
There was a problem hiding this comment.
like zach's comment, i think a todo comment would be nice
OussamaSaoudi
force-pushed
the
fix_main_tests
branch
from
0014c14 to
257cbe7
Compare
…#1135) ## What changes are proposed in this pull request? This PR fixes a test that fails on an error message to work across all arrow versions. the error message depends on the Debug print of arrow type. Due to this [commit](apache/arrow-rs#7469), the debug print result differs between arrow versions. ## How was this change tested? existing unit tests. Indentation was tested by breaking a test and checking that the error message is easy to read.
3 participants
What changes are proposed in this pull request?
This PR fixes a test that fails on an error message to work across all arrow versions. the error message depends on the Debug print of arrow type. Due to this commit, the debug print result differs between arrow versions.
How was this change tested?
existing unit tests.
Indentation was tested by breaking a test and checking that the error message is easy to read.