Make tests more idiomatic (#212)

* Reduce `ok().expect()` to equivalent `.expect()`

* Drop redundant imports in tests

* Use bytes

* Drop unused references and mutable variables

* Drop unused lifetimes

* Use arrays over vectors

* Safe type conversion

* Fix float comparison

Co-authored-by: Roman Valls Guimera <brainstorm@users.noreply.github.com>

Author: juliangehring

src/bam/ext.rs

@@ -4304,10 +4304,10 @@ mod tests {
 
         //
         //for the rest, we just verify that they have the expected amount of None in each position
-        fn some_count(pairs: &Vec<[Option<i64>; 2]>, pos: usize) -> i64 {
+        fn some_count(pairs: &[[Option<i64>; 2]], pos: usize) -> i64 {
             pairs.iter().filter(|x| x[pos].is_some()).count() as i64
         }
-        fn none_count(pairs: &Vec<[Option<i64>; 2]>, pos: usize) -> i64 {
+        fn none_count(pairs: &[[Option<i64>; 2]], pos: usize) -> i64 {
             pairs.iter().filter(|x| x[pos].is_none()).count() as i64
         }
 
@@ -9132,7 +9132,7 @@ mod tests {
         }
 
         let mut read = bam::Record::new();
-        for (input_cigar, supposed_length_wo_hard_clip, supposed_length_with_hard_clip) in vec![
+        for (input_cigar, supposed_length_wo_hard_clip, supposed_length_with_hard_clip) in &[
             ("40M", 40, 40),
             ("40=", 40, 40),
             ("40X", 40, 40),
@@ -9146,13 +9146,16 @@ mod tests {
         ] {
             read.set(
                 b"test",
-                Some(&CigarString::try_from(input_cigar).unwrap()),
+                Some(&CigarString::try_from(*input_cigar).unwrap()),
                 b"agtc",
                 b"BBBB",
             );
-            assert_eq!(read.seq_len_from_cigar(false), supposed_length_wo_hard_clip);
             assert_eq!(
-                read.seq_len_from_cigar(true),
+                &read.seq_len_from_cigar(false),
+                supposed_length_wo_hard_clip
+            );
+            assert_eq!(
+                &read.seq_len_from_cigar(true),
                 supposed_length_with_hard_clip
             );
         }

src/bam/mod.rs

@@ -946,7 +946,6 @@ mod tests {
     use std::fs;
     use std::path::Path;
     use std::str;
-    use tempdir;
 
     fn gold() -> (
         [&'static [u8]; 6],
@@ -1003,7 +1002,7 @@ CCCCCCCCCCCCCCCCCCC"[..],
         (names, flags, seqs, quals, cigars)
     }
 
-    fn compare_inner_bam_cram_records(cram_records: &Vec<Record>, bam_records: &Vec<Record>) {
+    fn compare_inner_bam_cram_records(cram_records: &[Record], bam_records: &[Record]) {
         // Selectively compares bam1_t struct fields from BAM and CRAM
         for (c1, b1) in cram_records.iter().zip(bam_records.iter()) {
             // CRAM vs BAM l_data is off by 3, see: https://github.com/rust-bio/rust-htslib/pull/184#issuecomment-590133544
@@ -1033,7 +1032,7 @@ CCCCCCCCCCCCCCCCCCC"[..],
 
         for (i, record) in bam.records().enumerate() {
             let rec = record.expect("Expected valid record");
-            println!("{}", str::from_utf8(rec.qname()).ok().unwrap());
+            println!("{}", str::from_utf8(rec.qname()).unwrap());
             assert_eq!(rec.qname(), names[i]);
             assert_eq!(rec.flags(), flags[i]);
             assert_eq!(rec.seq().as_bytes(), seqs[i]);
@@ -1066,9 +1065,7 @@ CCCCCCCCCCCCCCCCCCC"[..],
 
     #[test]
     fn test_seek() {
-        let mut bam = Reader::from_path(&Path::new("test/test.bam"))
-            .ok()
-            .expect("Error opening file.");
+        let mut bam = Reader::from_path(&Path::new("test/test.bam")).expect("Error opening file.");
 
         let mut names_by_voffset = HashMap::new();
 
@@ -1088,16 +1085,14 @@ CCCCCCCCCCCCCCCCCCC"[..],
             println!("{} {}", offset, qname);
             bam.seek(*offset).unwrap();
             bam.read(&mut rec).unwrap();
-            let rec_qname = str::from_utf8(rec.qname()).ok().unwrap().to_string();
+            let rec_qname = str::from_utf8(rec.qname()).unwrap().to_string();
             assert_eq!(qname, &rec_qname);
         }
     }
 
     #[test]
     fn test_read_sam_header() {
-        let bam = Reader::from_path(&"test/test.bam")
-            .ok()
-            .expect("Error opening file.");
+        let bam = Reader::from_path(&"test/test.bam").expect("Error opening file.");
 
         let true_header = "@SQ\tSN:CHROMOSOME_I\tLN:15072423\n@SQ\tSN:CHROMOSOME_II\tLN:15279345\
              \n@SQ\tSN:CHROMOSOME_III\tLN:13783700\n@SQ\tSN:CHROMOSOME_IV\tLN:17493793\n@SQ\t\
@@ -1116,19 +1111,15 @@ CCCCCCCCCCCCCCCCCCC"[..],
         assert!(bam.header.target_len(tid_1).expect("Expected target len.") == 15072423);
 
         // fetch to position containing reads
-        bam.fetch(tid_1, 0, 2)
-            .ok()
-            .expect("Expected successful fetch.");
+        bam.fetch(tid_1, 0, 2).expect("Expected successful fetch.");
         assert!(bam.records().count() == 6);
 
         // compare reads
-        bam.fetch(tid_1, 0, 2)
-            .ok()
-            .expect("Expected successful fetch.");
+        bam.fetch(tid_1, 0, 2).expect("Expected successful fetch.");
         for (i, record) in bam.records().enumerate() {
             let rec = record.expect("Expected valid record");
 
-            println!("{}", str::from_utf8(rec.qname()).ok().unwrap());
+            println!("{}", str::from_utf8(rec.qname()).unwrap());
             assert_eq!(rec.qname(), names[i]);
             assert_eq!(rec.flags(), flags[i]);
             assert_eq!(rec.seq().as_bytes(), seqs[i]);
@@ -1147,18 +1138,16 @@ CCCCCCCCCCCCCCCCCCC"[..],
 
         // fetch to position containing reads
         bam.fetch_str(format!("{}:{}-{}", str::from_utf8(sq_1).unwrap(), 0, 2).as_bytes())
-            .ok()
             .expect("Expected successful fetch.");
         assert!(bam.records().count() == 6);
 
         // compare reads
         bam.fetch_str(format!("{}:{}-{}", str::from_utf8(sq_1).unwrap(), 0, 2).as_bytes())
-            .ok()
             .expect("Expected successful fetch.");
         for (i, record) in bam.records().enumerate() {
             let rec = record.expect("Expected valid record");
 
-            println!("{}", str::from_utf8(rec.qname()).ok().unwrap());
+            println!("{}", str::from_utf8(rec.qname()).unwrap());
             assert_eq!(rec.qname(), names[i]);
             assert_eq!(rec.flags(), flags[i]);
             assert_eq!(rec.seq().as_bytes(), seqs[i]);
@@ -1177,9 +1166,7 @@ CCCCCCCCCCCCCCCCCCC"[..],
 
     #[test]
     fn test_read_indexed() {
-        let bam = IndexedReader::from_path(&"test/test.bam")
-            .ok()
-            .expect("Expected valid index.");
+        let bam = IndexedReader::from_path(&"test/test.bam").expect("Expected valid index.");
         _test_read_indexed_common(bam);
     }
     #[test]
@@ -1188,7 +1175,6 @@ CCCCCCCCCCCCCCCCCCC"[..],
             &"test/test_different_index_name.bam",
             &"test/test.bam.bai",
         )
-        .ok()
         .expect("Expected valid index.");
         _test_read_indexed_common(bam);
     }
@@ -1268,7 +1254,7 @@ CCCCCCCCCCCCCCCCCCC"[..],
             assert_eq!(rec.aux(b"NM").unwrap(), Aux::Integer(15));
 
             // Equal length qname
-            assert!(rec.qname()[0] != 'X' as u8);
+            assert!(rec.qname()[0] != b'X');
             rec.set_qname(b"X");
             assert_eq!(rec.qname(), b"X");
 
@@ -1313,22 +1299,20 @@ CCCCCCCCCCCCCCCCCCC"[..],
                 .push_tag(b"SN", &"1")
                 .push_tag(b"LN", &10000000),
         );
-        let mut header = HeaderView::from_header(&_header);
+        let header = HeaderView::from_header(&_header);
 
         let line =
             b"blah1	0	1	1	255	1M	*	0	0	A	F	CB:Z:AAAA-1	UR:Z:AAAA	UB:Z:AAAA	GX:Z:G1	xf:i:1	fx:Z:G1\tli:i:0\ttf:Z:cC";
 
-        let mut rec = Record::from_sam(&mut header, line).unwrap();
+        let mut rec = Record::from_sam(&header, line).unwrap();
         assert_eq!(rec.qname(), b"blah1");
         rec.set_qname(b"r0");
         assert_eq!(rec.qname(), b"r0");
     }
 
     #[test]
     fn test_remove_aux() {
-        let mut bam = Reader::from_path(&Path::new("test/test.bam"))
-            .ok()
-            .expect("Error opening file.");
+        let mut bam = Reader::from_path(&Path::new("test/test.bam")).expect("Error opening file.");
 
         for record in bam.records() {
             let mut rec = record.expect("Expected valid record");
@@ -1352,9 +1336,7 @@ CCCCCCCCCCCCCCCCCCC"[..],
     fn test_write() {
         let (names, _, seqs, quals, cigars) = gold();
 
-        let tmp = tempdir::TempDir::new("rust-htslib")
-            .ok()
-            .expect("Cannot create temp dir");
+        let tmp = tempdir::TempDir::new("rust-htslib").expect("Cannot create temp dir");
         let bampath = tmp.path().join("test.bam");
         println!("{:?}", bampath);
         {
@@ -1367,22 +1349,19 @@ CCCCCCCCCCCCCCCCCCC"[..],
                 ),
                 Format::BAM,
             )
-            .ok()
             .expect("Error opening file.");
 
             for i in 0..names.len() {
                 let mut rec = record::Record::new();
                 rec.set(names[i], Some(&cigars[i]), seqs[i], quals[i]);
                 rec.push_aux(b"NM", &Aux::Integer(15));
 
-                bam.write(&mut rec).expect("Failed to write record.");
+                bam.write(&rec).expect("Failed to write record.");
             }
         }
 
         {
-            let mut bam = Reader::from_path(&bampath)
-                .ok()
-                .expect("Error opening file.");
+            let mut bam = Reader::from_path(&bampath).expect("Error opening file.");
 
             for i in 0..names.len() {
                 let mut rec = record::Record::new();
@@ -1403,9 +1382,7 @@ CCCCCCCCCCCCCCCCCCC"[..],
     fn test_write_threaded() {
         let (names, _, seqs, quals, cigars) = gold();
 
-        let tmp = tempdir::TempDir::new("rust-htslib")
-            .ok()
-            .expect("Cannot create temp dir");
+        let tmp = tempdir::TempDir::new("rust-htslib").expect("Cannot create temp dir");
         let bampath = tmp.path().join("test.bam");
         println!("{:?}", bampath);
         {
@@ -1418,7 +1395,6 @@ CCCCCCCCCCCCCCCCCCC"[..],
                 ),
                 Format::BAM,
             )
-            .ok()
             .expect("Error opening file.");
             bam.set_threads(4).unwrap();
 
@@ -1429,14 +1405,12 @@ CCCCCCCCCCCCCCCCCCC"[..],
                 rec.push_aux(b"NM", &Aux::Integer(15));
                 rec.set_pos(i as i64);
 
-                bam.write(&mut rec).expect("Failed to write record.");
+                bam.write(&rec).expect("Failed to write record.");
             }
         }
 
         {
-            let mut bam = Reader::from_path(&bampath)
-                .ok()
-                .expect("Error opening file.");
+            let mut bam = Reader::from_path(&bampath).expect("Error opening file.");
 
             for (i, _rec) in bam.records().enumerate() {
                 let idx = i % names.len();
@@ -1460,36 +1434,27 @@ CCCCCCCCCCCCCCCCCCC"[..],
         // Verify that BAM headers are transmitted correctly when using an existing BAM as a
         // template for headers.
 
-        let tmp = tempdir::TempDir::new("rust-htslib")
-            .ok()
-            .expect("Cannot create temp dir");
+        let tmp = tempdir::TempDir::new("rust-htslib").expect("Cannot create temp dir");
         let bampath = tmp.path().join("test.bam");
         println!("{:?}", bampath);
 
-        let mut input_bam = Reader::from_path(&"test/test.bam")
-            .ok()
-            .expect("Error opening file.");
+        let mut input_bam = Reader::from_path(&"test/test.bam").expect("Error opening file.");
 
         {
             let mut bam = Writer::from_path(
                 &bampath,
                 &Header::from_template(&input_bam.header()),
                 Format::BAM,
             )
-            .ok()
             .expect("Error opening file.");
 
             for rec in input_bam.records() {
-                bam.write(&rec.unwrap())
-                    .ok()
-                    .expect("Failed to write record.");
+                bam.write(&rec.unwrap()).expect("Failed to write record.");
             }
         }
 
         {
-            let copy_bam = Reader::from_path(&bampath)
-                .ok()
-                .expect("Error opening file.");
+            let copy_bam = Reader::from_path(&bampath).expect("Error opening file.");
 
             // Verify that the header came across correctly
             assert_eq!(input_bam.header().as_bytes(), copy_bam.header().as_bytes());
@@ -1502,9 +1467,7 @@ CCCCCCCCCCCCCCCCCCC"[..],
     fn test_pileup() {
         let (_, _, seqs, quals, _) = gold();
 
-        let mut bam = Reader::from_path(&"test/test.bam")
-            .ok()
-            .expect("Error opening file.");
+        let mut bam = Reader::from_path(&"test/test.bam").expect("Error opening file.");
         let pileups = bam.pileup();
         for pileup in pileups.take(26) {
             let _pileup = pileup.expect("Expected successful pileup.");
@@ -1523,9 +1486,7 @@ CCCCCCCCCCCCCCCCCCC"[..],
 
     #[test]
     fn test_idx_pileup() {
-        let mut bam = IndexedReader::from_path(&"test/test.bam")
-            .ok()
-            .expect("Error opening file.");
+        let mut bam = IndexedReader::from_path(&"test/test.bam").expect("Error opening file.");
         // read without fetch
         for pileup in bam.pileup() {
             pileup.unwrap();
@@ -1555,7 +1516,7 @@ CCCCCCCCCCCCCCCCCCC"[..],
 
         let sam_recs: Vec<Record> = sam
             .split(|x| *x == b'\n')
-            .filter(|x| x.len() > 0 && x[0] != b'@')
+            .filter(|x| !x.is_empty() && x[0] != b'@')
             .map(|line| Record::from_sam(rdr.header(), line).unwrap())
             .collect();
 
@@ -1569,9 +1530,7 @@ CCCCCCCCCCCCCCCCCCC"[..],
         // test the cached and uncached ways of getting the cigar string.
 
         let (_, _, _, _, cigars) = gold();
-        let mut bam = Reader::from_path(&Path::new("test/test.bam"))
-            .ok()
-            .expect("Error opening file.");
+        let mut bam = Reader::from_path(&Path::new("test/test.bam")).expect("Error opening file.");
 
         for (i, record) in bam.records().enumerate() {
             let rec = record.expect("Expected valid record");
@@ -1619,9 +1578,7 @@ CCCCCCCCCCCCCCCCCCC"[..],
         let bam_records: Vec<Record> = bam_reader.records().map(|v| v.unwrap()).collect();
 
         // New CRAM file
-        let tmp = tempdir::TempDir::new("rust-htslib")
-            .ok()
-            .expect("Cannot create temp dir");
+        let tmp = tempdir::TempDir::new("rust-htslib").expect("Cannot create temp dir");
         let cram_path = tmp.path().join("test.cram");
 
         // Write BAM records to new CRAM file
@@ -1655,15 +1612,13 @@ CCCCCCCCCCCCCCCCCCC"[..],
             );
 
             let mut cram_writer = Writer::from_path(&cram_path, &header, Format::CRAM)
-                .ok()
                 .expect("Error opening CRAM file.");
             cram_writer.set_reference(ref_path).unwrap();
 
             // Write BAM records to CRAM file
             for rec in bam_records.iter() {
                 cram_writer
                     .write(&rec)
-                    .ok()
                     .expect("Faied to write record to CRAM.");
             }
         }
@@ -1766,7 +1721,7 @@ CCCCCCCCCCCCCCCCCCC"[..],
 
     #[test]
     fn test_sam_writer_example() {
-        fn from_bam_with_filter<'a, 'b, F>(bamfile: &'a str, samfile: &'b str, f: F) -> bool
+        fn from_bam_with_filter<F>(bamfile: &str, samfile: &str, f: F) -> bool
         where
             F: Fn(&record::Record) -> Option<bool>,
         {
@@ -1782,7 +1737,7 @@ CCCCCCCCCCCCCCCCCCC"[..],
                     None => return true,
                     Some(false) => {}
                     Some(true) => {
-                        if let Err(_) = sam_writer.write(&parsed) {
+                        if sam_writer.write(&parsed).is_err() {
                             return false;
                         }
                     }