Add many tests / Remove unused codes about `slurp` fasta #60

ayamada · 2017-04-19T07:37:16Z

Add many tests that made from checking coverage results. Individual commits are in https://github.com/ayamada/cljam/commits/feature/add-tests-for-coverage-only-fixes-2 .
Remove obsoleted slurp fn in fasta (Refer Activate cljam.fasta/slurp #58 (comment) )

…s-ci)

codecov-io · 2017-04-19T08:19:38Z

Codecov Report

Merging #60 into master will increase coverage by 8.53%.
The diff coverage is n/a.

@@            Coverage Diff            @@
##           master     #60      +/-   ##
=========================================
+ Coverage   73.87%   82.4%   +8.53%     
=========================================
  Files          56      56              
  Lines        3288    3285       -3     
  Branches      334     363      +29     
=========================================
+ Hits         2429    2707     +278     
+ Misses        525     215     -310     
- Partials      334     363      +29

Impacted Files	Coverage Δ
src/cljam/fasta/reader.clj	`89.18% <ø> (+18.91%)`	⬆️
src/cljam/fasta/core.clj	`100% <ø> (+23.52%)`	⬆️
src/cljam/pileup.clj	`93.33% <0%> (-6.67%)`	⬇️
src/cljam/bam_index/writer.clj	`74.87% <0%> (+1.47%)`	⬆️
src/cljam/bam_index/core.clj	`71.92% <0%> (+1.75%)`	⬆️
src/cljam/tabix.clj	`100% <0%> (+3.92%)`	⬆️
src/cljam/bam/writer.clj	`76.86% <0%> (+4.47%)`	⬆️
src/cljam/twobit.clj	`78.21% <0%> (+4.95%)`	⬆️
src/cljam/sam/writer.clj	`70% <0%> (+5%)`	⬆️
src/cljam/util.clj	`78.87% <0%> (+5.63%)`	⬆️
... and 19 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e365309...e566fee. Read the comment docs.

alumi · 2017-04-19T09:53:01Z

test/cljam/t_dedupe.clj

+    (let [in-file test-sorted-bam-file
+          out-file (str temp-dir "/deduped.bam")]
+      (is (not-throw? (dedupe/dedupe in-file out-file)))
+      ;; TODO: check alignments of out-file


Could you open issues for TODOs in this diff?
Coverage will be increased but almost no code semantics is tested here.
I think we need to track the problem to remind that.

Yes, I will open issue for this, later.
Some tests needs to check result data certainly, but I cannot create those data.

alumi · 2017-04-19T09:56:16Z

test/cljam/t_bam.clj

+  (with-before-after {:before (prepare-cache!)
+                      :after (clean-cache!)}
+    (let [rdr (bam/reader test-sorted-bam-file :ignore-index false)]
+      ;; TODO: Does not works?


Please open an issue for here, too.

I think representing entire region of a single chromosome by {:chr REFNAME} will be useful.

All right, I will open issue for this, too.
I guess I wrong something, but I didn't know this reason.

totakke

Thank you for great contribution. I've added some comments. Awaiting for your fix or response.

totakke · 2017-04-19T11:57:14Z

test/cljam/t_bam.clj

+          true
+          (when (and (= (:rname aln1) (:rname aln2))
+                     (= (:pos aln1) (:pos aln2)))
+            (recur (rest alns1) (rest alns2))))))))


This implementation using loop/recur is complicated. I think

(defn- shallow= [alns1 alns2] (= (map #(selece-keys % [:rname :pos]) alns1) (map #(selece-keys % [:rname :pos]) alns2)))

is more simple. Or are there any reason? (memory usage?). Also pointer=.

Thanks for review.
I guess sure too, I fixed 88ff657 .

totakke · 2017-04-19T12:04:43Z

test/cljam/t_bam.clj

 (deftest bamreader
  (with-before-after {:before (do (prepare-cache!)
                                  (spit-bam-for-test temp-file test-sam))
                      :after (clean-cache!)}
-    (let [rdr (bam/reader temp-file :ignore-index true)]
-      (is (= (io/read-refs rdr) test-sam-refs)))))
+    (let [rdr (bam/reader temp-file :ignore-index false)]


BAMReader will not be closed. with-open should be used instead of let. (I don't know why original code uses let...)

I agree, fixed 91ae96e .

totakke · 2017-04-19T12:10:07Z

test/cljam/t_bam_indexer.clj

+      (is (= (count (io/read-alignments r {:chr "chr1" :start 23000000 :end 24000000 :depth :deep}))
+             10010))
+      (is (= (count (io/read-alignments r {:chr "chr1" :start 23000000 :end 23500000 :depth :deep}))
+             3806))


are is better than is because similar codes are repeated.

I fixed e566fee .

totakke · 2017-04-19T12:12:11Z

test/cljam/t_cli.clj


 (defmacro with-out-file
  [f & body]
-  `(binding [*out* (clojure.java.io/writer ~f)]
-     ~@body))
+  `(let [os# (clojure.java.io/output-stream ~f)


io/output-stream

Fixed cf24f13 .

totakke · 2017-04-19T12:12:28Z

test/cljam/t_cli.clj

+         ps# (PrintStream. os#)]
+     (try
+       (System/setOut ps#)
+       (binding [*out* (clojure.java.io/writer os#)]


io/writer

totakke · 2017-04-19T12:17:16Z

test/cljam/t_fasta.clj

+  (is (thrown? java.io.IOException
+               (let [rdr (fasta/reader test-tabix-file)]
+                 (fasta/read rdr))))
+  (let [rdr (fasta/reader test-fa-file)]


with-open instead of let

totakke · 2017-04-19T12:21:35Z

test/cljam/t_util.clj

+  (is (= (util/str->int "-456") -456))
+  (is (= (util/str->int "+789") 789))
+  (is (= Integer (type (util/str->int "123"))))
+  (is (= Long (type (util/str->int "12345678901"))))


I feel (instance? Long (util/str->int "12345678901")) is better.

Fixed 74f19e4 .

Thanks for your reviews!

ayamada · 2017-04-20T05:03:20Z

I fixed above problems, are these OK ?

totakke · 2017-04-20T05:25:07Z

Perfect, thanks.

ayamada · 2017-04-20T07:15:40Z

Thanks for merge!
I created related issues #61 and #62 .

ayamada added 3 commits April 19, 2017 16:22

Add many tests

3abd4ea

Remove unused codes about slurp fasta

851528b

Remove one invalid file test (because throw OutOfMemoryError in travi…

98bb95d

…s-ci)

alumi reviewed Apr 19, 2017

View reviewed changes

alumi approved these changes Apr 19, 2017

View reviewed changes

totakke requested changes Apr 19, 2017

View reviewed changes

ayamada added 5 commits April 20, 2017 04:26

Refine to check type

74f19e4

Refine code to check equivalence fn

88ff657

Trim unnecessary fully qualified namespace

cf24f13

Use with-open instead of let, for Closeable

91ae96e

Replace from repeated similar codes of is to are

e566fee

totakke approved these changes Apr 20, 2017

View reviewed changes

totakke merged commit ff91c66 into chrovis:master Apr 20, 2017

This was referenced Apr 20, 2017

Add tests to check generated data correctness #61

Closed

May need to fix a test for bam-reader with :chr argument #62

Closed

ayamada deleted the feature/add-tests-for-coverage-only-fixes-s branch April 28, 2017 15:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add many tests / Remove unused codes about `slurp` fasta #60

Add many tests / Remove unused codes about `slurp` fasta #60

ayamada commented Apr 19, 2017

codecov-io commented Apr 19, 2017 •

edited

Loading

alumi Apr 19, 2017

ayamada Apr 19, 2017

alumi Apr 19, 2017

ayamada Apr 19, 2017

totakke left a comment

totakke Apr 19, 2017

ayamada Apr 20, 2017

totakke Apr 19, 2017

ayamada Apr 20, 2017

totakke Apr 19, 2017

ayamada Apr 20, 2017

totakke Apr 19, 2017

ayamada Apr 20, 2017

totakke Apr 19, 2017

totakke Apr 19, 2017

totakke Apr 19, 2017

ayamada Apr 20, 2017

ayamada commented Apr 20, 2017

totakke commented Apr 20, 2017

ayamada commented Apr 20, 2017

Add many tests / Remove unused codes about slurp fasta #60

Add many tests / Remove unused codes about slurp fasta #60

Conversation

ayamada commented Apr 19, 2017

codecov-io commented Apr 19, 2017 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

totakke left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ayamada commented Apr 20, 2017

totakke commented Apr 20, 2017

ayamada commented Apr 20, 2017

Add many tests / Remove unused codes about `slurp` fasta #60

Add many tests / Remove unused codes about `slurp` fasta #60

codecov-io commented Apr 19, 2017 •

edited

Loading