Fix lightning-persister tests for upcoming rustc changes #1033
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This should catch any platform-specific behavior changes in rustc before they land in stable.
Codecov Report
@@ Coverage Diff @@
## main #1033 +/- ##
=======================================
Coverage 90.80% 90.81%
=======================================
Files 60 60
Lines 31460 31463 +3
=======================================
+ Hits 28568 28573 +5
+ Misses 2892 2890 -2
Continue to review full report at Codecov.
|
jkczyz
reviewed
Aug 2, 2021
| @@ -135,7 +135,7 @@ mod tests { | |||
| // Create the channel data file and make it a directory. | |||
| fs::create_dir_all(get_full_filepath(path.clone(), filename.to_string())).unwrap(); | |||
| match write_to_file(path.clone(), filename.to_string(), &test_writeable) { | |||
| Err(e) => assert_eq!(e.kind(), io::ErrorKind::Other), | |||
| Err(e) => assert_eq!(e.raw_os_error(), Some(libc::EISDIR)), | |||
There was a problem hiding this comment.
Consider adding a TODO to switch back to kind once possible. Though I guess that is a long way out. 😕
There was a problem hiding this comment.
Yea, that's gonna require at least rustc-now, so I guess in at least two years per the rust-bitcoin MSRV policy, assuming distros upgrade. I think this is fine, in any case, at least it won't break.
TheBlueMatt
force-pushed
the
2021-07-fix-beta
branch
from
d25a27a to
bef5402
Compare
lightning-block-sync/src/http.rs
Outdated
| @@ -636,7 +636,7 @@ pub(crate) mod client_tests { | |||
| #[test] | |||
| fn connect_to_unresolvable_host() { | |||
| match HttpClient::connect(("example.invalid", 80)) { | |||
| Err(e) => assert_eq!(e.kind(), std::io::ErrorKind::Other), | |||
| Err(_) => { /* rust libstd currently doesn't have a specific error for Host Not Found for us to match against */ }, | |||
There was a problem hiding this comment.
Thoughts on this patch or similar:
diff --git a/lightning-block-sync/src/http.rs b/lightning-block-sync/src/http.rs
index 1f30bea1..93f7bafb 100644
--- a/lightning-block-sync/src/http.rs
+++ b/lightning-block-sync/src/http.rs
@@ -636,7 +636,9 @@ pub(crate) mod client_tests {
#[test]
fn connect_to_unresolvable_host() {
match HttpClient::connect(("example.invalid", 80)) {
- Err(_) => { /* rust libstd currently doesn't have a specific error for Host Not Found for us to match against */ },
+ Err(e) => {
+ assert_eq!(e.to_string(), "failed to lookup address information: Name or service not known");
+ },
Ok(_) => panic!("Expected error"),
}
}
There was a problem hiding this comment.
Good point. I went with e.to_string().contains("Name or service not known") as I'm not 100% sure to_string is guaranteed to never change, either.
There was a problem hiding this comment.
Turns out its not cross-platform. I think failed to lookup address information will be, for now, though.
TheBlueMatt
force-pushed
the
2021-07-fix-beta
branch
2 times, most recently
from
3b328ee to
9d10637
Compare
std::io::ErrorKind is a `#[non_exhaustive]` enum as more specific error types are to be added in the future. It was unclear in the docs until very recently, however, that this is to be done by re-defining `ErrorKind::Other` errors to new enum variants. Thus, our tests which check explicitly for `ErrorKind::Other` as a result of trying to access a directory as a file were incorrect. Sadly, these generated no meaningful feedback from rustc at all, except that they're suddenly failing in rustc beta! After some back-and-forth, it seems rustc is moving forward breaking existing code in future versions, so we move to the "correct" check here, which is to check the raw IO error. See rust-lang/rust#86442 and rust-lang/rust#85746 for more info.
TheBlueMatt
force-pushed
the
2021-07-fix-beta
branch
from
9d10637 to
8490944
Compare
valentinewallace
approved these changes
Aug 2, 2021
jkczyz
approved these changes
Aug 3, 2021
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Upcoming versions of rustc will break existing code (:scream:), so we need to tweak two tests in
lightning-persisterto work around this. This should fix tests on rustc-beta.