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

assets::tests::syntax_detection_invalid_utf8 failed with 0.15.1 #983

Closed
lilyball opened this issue May 11, 2020 · 8 comments · Fixed by #985
Closed

assets::tests::syntax_detection_invalid_utf8 failed with 0.15.1 #983

lilyball opened this issue May 11, 2020 · 8 comments · Fixed by #985
Labels
bug Something isn't working

Comments

@lilyball
Copy link

I just tried compiling bat 0.15.1 via Nix on macOS and it failed during the tests phase:

---- assets::tests::syntax_detection_invalid_utf8 stdout ----
thread 'assets::tests::syntax_detection_invalid_utf8' panicked at 'called `Result::unwrap()` on an `Err` value: Os { code: 92, kind: Other, message: "Illegal byte sequence" }', src/assets.rs:287:37
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace


failures:
    assets::tests::syntax_detection_invalid_utf8

Also of note, the color escapes used by the test runner all stuck a visible (B after each colored string. Which is to say, each "ok" was "ok(B" and the "FAILED" was "FAILED(B". Colors printed by rustc itself were fine. I don't know if this is meaningful or just weird though.

@lilyball lilyball added the bug Something isn't working label May 11, 2020
@lilyball
Copy link
Author

The test failure also occurs when running cargo test on a manual checkout outside of Nix. The (B oddity doesn't so I guess that's unrelated.

@lilyball
Copy link
Author

I suspect this is a test that simply always fails on macOS. It appears to be attempting to create a filename with invalid utf8, but macOS doesn't allow this. This test will need to be disabled for macOS.

@lilyball
Copy link
Author

The following diff fixes cargo test:

diff --git a/src/assets.rs b/src/assets.rs
index 4f8556f..222abc2 100644
--- a/src/assets.rs
+++ b/src/assets.rs
@@ -336,7 +336,7 @@ mod tests {
         assert_eq!(test.syntax_for_file("Makefile"), "Makefile");
     }
 
-    #[cfg(unix)]
+    #[cfg(all(unix,not(target_os = "macos")))]
     #[test]
     fn syntax_detection_invalid_utf8() {
         use std::os::unix::ffi::OsStrExt;

However it does introduce a warning

warning: method is never used: `syntax_for_file_os`
   --> src/assets.rs:301:9
    |
301 |         fn syntax_for_file_os(&self, file_name: &OsStr) -> String {
    |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    |
    = note: `#[warn(dead_code)]` on by default

I don't know if it's better to cfg-out that function on macos too, or just mark it with #[allow(dead_code)].

@lilyball
Copy link
Author

I've included the above patch in the Nix PR for 0.15.1.

@eth-p
Copy link
Collaborator

eth-p commented May 12, 2020

Disabling the tests on MacOS is a working solution, but I felt like it still left some room for error (e.g. Linux with ZFS utf8only). I made a PR (#985) that attempts to fix the underlying cause of this error by avoiding accessing the filesystem for some of the testing.

@sharkdp
Copy link
Owner

sharkdp commented May 12, 2020

I suspect this is a test that simply always fails on macOS. It appears to be attempting to create a filename with invalid utf8, but macOS doesn't allow this.

Oh right. I ran into this with fd as well. I wonder why this hasn't been caught in a CI build (https://travis-ci.org/github/sharkdp/bat/builds/685790785)? Maybe the Travis OSX VMs use a file system that allows for invalid UTF-8 filenames...

Disabling the tests on MacOS is a working solution, but I felt like it still left some room for error (e.g. Linux with ZFS utf8only).

So how about just letting the test succeed in case the file could not be created? I think that would be a reasonable solution which would work on all operating systems.

I made a PR (#985) that attempts to fix the underlying cause of this error by avoiding accessing the filesystem for some of the testing.

Thanks. Let's discuss these changes in the PR.

@lilyball
Copy link
Author

Maybe the Travis OSX VMs use a file system that allows for invalid UTF-8 filenames...

It looks like HFS+ allows invalid UTF-8 by transforming the filename. The name invalid_\xFEutf8_filename.rs ends up on-disk as a file named invalid_%FEutf8_filename.rs instead. APFS doesn't do this, it just rejects the filename.

eth-p added a commit that referenced this issue May 15, 2020
Unify syntax detection for all variants of InputKind, fix #983
@lilyball
Copy link
Author

Confirmed fixed, thank you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants