-
Notifications
You must be signed in to change notification settings - Fork 294
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
increase filter_abund.py coverage #568
Conversation
@mr-c ready to merge |
@@ -1,3 1,8 @@ | |||
2014-07-24 Michael Wright <[email protected]> | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
two spaces after your name
You might like http://www.vim.org/scripts/script.php?script_id=1631
@wrightmhw Fix these two things ASAP and I'll include this in the next release; thanks! |
This is being bumped to the 1.2 release. |
ping @wrightmhw? |
9cfbfdb
to
0e8d31f
Compare
@mr-c I don't see how Jenkins is failing. All tests passed and no pylint/pep8 errors?? |
test this please |
|
||
args = [n_counting_ht, n_infile, '-o' 'outfile'] | ||
utils.runscript(script, args, in_dir) | ||
assert os.path.exists(outfile), outfile |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mr-c Is that assert testing the last set of args correctly, or is it testing a previous set?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch: If you're rerunning within the same test you should use a new filename especially when just doing existence checks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Umm, note that in args its using '-o outfile' and not '-o %s' % outfile, so you're literally writing to the file named 'outfile' not the file named by the value in the variable outfile...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ctb should I use %s or '.format' here?
http://stackoverflow.com/questions/12382719/python-way-of-printing-with-format-or-percent-form
(I know stack overflow, sorry. But I couldn't find documentation on my question. I only see it as important if '%' is not supported for python3)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On Tue, Sep 30, 2014 at 11:54:53PM -0700, Michael Wright wrote:
assert len(seqs) == 1, seqs assert 'GGTTGACGGGGCTCAGGG' in seqs
- args = [n_counting_ht, n_infile]
- utils.runscript(script, args, n_in_dir)
- seqs = set([r.sequence for r in screed.open(n_infile)])
- assert os.path.exists(n_outfile), n_outfile
- args = [n_counting_ht, n_infile, '-o' 'outfile']
- utils.runscript(script, args, in_dir)
- assert os.path.exists(outfile), outfile
@ctb should I use %s or '.format' here?
http://stackoverflow.com/questions/12382719/python-way-of-printing-with-format-or-percent-form
(I know stack overflow, sorry. But I couldn't find documentation on my question. I only see it as important if '%' is not supported for python3)
They've been mixed in the codebase, so it doesn't matter. I gather {} is
more modern.
1025900
to
a1cb6bc
Compare
Test PASSed. |
1 similar comment
Test PASSed. |
Test FAILed. |
2aa2bb8
to
854687a
Compare
Test FAILed. |
Test PASSed. |
Ping @wrightmhw. |
c578dfa
to
a35e7f9
Compare
I've rebased again the master tree and fixed the test. |
I can confirm that this PR increases the code coverage of |
increase filter_abund.py coverage
Check for code coverage.
pep8
/pylint
,cppcheck
, andmake doc
output. Useautopep8
andastyle -A10 --max-code-length=80
if needed.
were made?