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

Coverage audit #1236

Merged
merged 4 commits into from Aug 10, 2015
Merged

Coverage audit #1236

merged 4 commits into from Aug 10, 2015

Conversation

bocajnotnef
Copy link
Contributor

Scanned through the coverage report on master. Am adding tests or deleting unreachable code.

@bocajnotnef
Copy link
Contributor Author

Do we need the "debugging support" stuff in partition-graph? It never gets tested.

if "Linux" == platform.system():
    def __debug_vm_usage(msg):
        print("===> DEBUG: " + msg, file=sys.stderr)
        for vmstat in re.findall(r".*Vm.*", file("/proc/self/status").read()):
            print(vmstat, file=sys.stderr)
else:
    def __debug_vm_usage(msg):  # pylint: disable=unused-argument
        pass

https://github.com/dib-lab/khmer/blob/master/scripts/partition-graph.py#L32

@bocajnotnef
Copy link
Contributor Author

Said debug code is also in do-partition.

@bocajnotnef
Copy link
Contributor Author

  • Is it mergeable?
  • Did it pass the tests?
  • If it introduces new functionality in scripts/ is it tested?
    Check for code coverage with make clean diff-cover
  • Is it well formatted? Look at make pep8, make diff_pylint_report,
    make cppcheck, and make doc output. Use make format and manual
    fixing as needed.
  • Did it change the command-line interface? Only additions are allowed
    without a major version increment. Changing file formats also requires a
    major version number increment.
  • Is it documented in the ChangeLog?
    http://en.wikipedia.org/wiki/Changelog#Format
  • Was a spellchecker run on the source code and documentation after
    changes were made?
  • Is the Copyright year up to date?

@bocajnotnef
Copy link
Contributor Author

@mr-c @luizirber Ready for merge

@bocajnotnef
Copy link
Contributor Author

Considering adding #1225 in...

@ctb
Copy link
Member

ctb commented Aug 9, 2015

Do you want to get this in 2.0?

Titus Brown, ctbrown@ucdavis.edu

On Aug 9, 2015, at 2:19 PM, Jake Fenton notifications@github.com wrote:

Considering adding #1225 in...


Reply to this email directly or view it on GitHub.

@bocajnotnef
Copy link
Contributor Author

Only if you want it there. I don't see it impacting userspace much so it's probably not necessary--just makes it easier for people developing.

@mr-c
Copy link
Contributor

mr-c commented Aug 9, 2015

Please drop the partition-graph debugging code, yes.

@mr-c
Copy link
Contributor

mr-c commented Aug 9, 2015

This LGTM, thanks!

@bocajnotnef
Copy link
Contributor Author

retest this please

bocajnotnef added a commit that referenced this pull request Aug 10, 2015
@bocajnotnef bocajnotnef merged commit ea4eb0a into master Aug 10, 2015
@ctb ctb deleted the tests/add_coverage branch January 21, 2017 16:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants