search

numenta / nupic.core-legacy

Implementation of core NuPIC algorithms in C++ (under construction)
http://numenta.org
GNU Affero General Public License v3.0
272 stars 278 forks source link

Fix coverage reporting #324

Open rhyolight opened 9 years ago

rhyolight commented 9 years ago

Coveralls is reporting 0% test coverage on nupic.core for the past several months.

https://coveralls.io/r/numenta

rhyolight commented 9 years ago

Raised priority because annoying.

utensil commented 8 years ago

I've noticed that the first PR with 0% coverage report was #228 , see https://coveralls.io/github/numenta/nupic.core?page=118

image

Ever since #228 was merged, the coverage of nupic.core:master became 0% , see https://coveralls.io/github/numenta/nupic.core?page=117

image

In #228 , @rhyolight had the following comment :

image

@rhyolight , did you know something causing 0% coverage report that explains why #228 was OK, or did you believe dropping to 0% is cause by the same reason that you mentioned in https://github.com/numenta/nupic/issues/1843 ?

From my preliminary observation, #228 is THE PR causing 0%(And I'll investigate the details). Am I missing something here?

utensil commented 8 years ago

It's quite clear now that #228 is exactly the cause of 0%.

ddc622bf99c886692f2b19892a56ecc70e8fa5de of #228 removed the following lines:

image

which removed the option NTA_COV_ENABLED.

NTA_COV_ENABLED was used to add flags to instrument debug-ish information into the built binary, so when running tests, gcov will be able to gather coverage information.

NTA_COV_ENABLED is used in .travis.yml (which survived to HEAD): https://github.com/numenta/nupic.core/blob/master/.travis.yml#L100

image

But because this option no longer exists, it has no effect.

utensil commented 8 years ago

I've also noticed the following discussion:

image

And I sincerely sorry that due to my personal time issue, I didn't spot this and report this in time and left 0% coverage for 1+ year.

utensil commented 8 years ago

While the cause has been identified, fixing it is not easy.

nupic.core has changed a lot since the time I integrated google test and coverall. It has advanced to become installable as a standalone library, and from what I observed, the distributed binary of nupic.core(as pip package nupic.bindings and on s3) is also built by travis-ci, so it has to be a release version.

Coverage instrument means a lot of stub code into the binary, contains a lot of information about files, functions, branches and lines, which will affect the binary size and performance. Thus in order to bring the coverage functionality back, we need an extra debug version.

The ability of compiling a debug version of nupic.core exists: the CMAKE_BUILD_TYPE option which survived to date. We could add coverage related compiler flags back when CMAKE_BUILD_TYPE is Debug. We also need to link the tests with the debug version of nupic.core.

The above approach feels not intuitive, vulnerable to future maintenance, and adds extra build time for each Linux build(coverage report is current only enable for Linux builds). Here's a better/cleaner way to do this:

rhyolight commented 8 years ago

@numenta/nupic-committers Any opinions to @utensil's comments above?

Also, welcome back, @utensil !

utensil commented 8 years ago

Also, welcome back, @utensil !

Thanks! :smile:

oxtopus commented 8 years ago

Is there any problem with building both debug and release versions? The debug version is used for tests and coverage and release is published.

scottpurdy commented 8 years ago

@mrcslws recently fixed the Debug build and may have an opinion on this.

utensil commented 8 years ago

oxtopus commented:

Is there any problem with building both debug and release versions? The debug version is used for tests and coverage and release is published.

Not really. My concern was mainly build time, I'm under the impression that we are short of it(at least for nupic), are we?

There's another downside of my suggestion: If we have only one build for coverage and it's for a specific OS/compiler combination, then we won't been able to notice the coverage drop when a PR is targeted at another combination. In this point of view, an extra build for only coverage won't be scalable in future.

To sum it up, I'll start working on this, taking the approach that "building both debug and release versions, and link the tests with the debug version".

utensil commented 8 years ago

Initial investigation shows complications regarding:

This means, "add a separate build(preferably clang under Linux) with a separate environment variable, to build debug+coverage version of nupic.core and run tests and report coverage to coveralls" is a better choice.

utensil commented 8 years ago

Size comparison of libnupic_core.a:

Release Debug Coverage
7.8M 44M 76M
utensil commented 8 years ago

Progress report: "coverage linking problem with SWIG and Pythons tests" has become a pain to my work on 324-fix-coverage. It seems that they don't get along very well, especially through the CMake wall. I failed to overcome this on at least one platform in a reasonable time(it's longer than a month now).

So I would try to disable SWIG related stuff in a separate coverage build for now.

utensil commented 8 years ago

Progress report:

I've been trying to learn more about SWIG and Python internals to develop understandings of the fatal failure of my work on this issue at https://github.com/utensil/nupic.core/tree/324-fix-coverage , in which I had pieced things together and bypass some fried circuits (see https://github.com/utensil/nupic.core/commit/0eaba6b523567d620f1cd756790aaff2ea391c62 ) but ended up :boom: ( core dumped , see https://travis-ci.org/utensil/nupic.core/jobs/127164838 ). And I was also stuck with work related stuff and had little time to work on it.

I hope to prepare a detailed description of the problem and ask for help from committers and the forum in the coming weeks.