Add Code Coverage #628

JP-Ellis · 2019-05-04T13:45:25Z

This pull request's primary goal is to add code coverage, which can be seen at codecov and coveralls (note that former only displays stats from the master branch on the front page, but you can see stats from commits at https://codecov.io/gh/JP-Ellis/ndarray/commits).

In addition, this pull requests also adds cargo clippy and cargo fmt. Failure to pass these does not cause the build to break, but serves as a warning.

I also took the slight liberty to change some of the features relating to testing. Nightly features are kept behind the nightly flag (which has no other dependencies), and cargo test automatically enables the cfg(test) flag making the tst feature somewhat redundant. Tests which are specific to certain features (such as blas) can be enabled by invoking cargo with the --feature blas flag.

Lastly, the first build does take a long time due to the cargo travis installation; however, I have enabled caching with travis so that subsequent runs can complete substantially faster.

EDIT: Forgot to mention that I've added the flags -D warnings and -D missing_docs which causes the build to fail if there are any warnings or missing documentation on public facing elements. Fortunately, there were very few such warnings which I've fixed (albeit temporarily in the case of the documentation).

codecov-io · 2019-05-04T14:36:28Z

Codecov Report

❗ No coverage uploaded for pull request base (master@ea82bf4). Click here to learn what that means.
The diff coverage is 100%.

@@           Coverage Diff           @@
##             master   #628   +/-   ##
=======================================
  Coverage          ?    96%           
=======================================
  Files             ?     78           
  Lines             ?   8851           
  Branches          ?      0           
=======================================
  Hits              ?   8497           
  Misses            ?    354           
  Partials          ?      0

Impacted Files	Coverage Δ
serialization-tests/tests/serialize.rs	`100% <100%> (ø)`

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 ea82bf4...5379aa1. Read the comment docs.

LukeMathWalker · 2019-05-04T14:49:30Z

Nice!
A couple of questions/comments:

there is an open PR for cargo fmt (Run rustfmt and add to travis #608) and I'd like to keep the same spirit: if the formatting check fails, I think we should fail the build.
I have seen you have added in a lot of "TODO: document" where I assume clippy was complaining about missing docs. Wouldn't it be better to keep those missing docs warnings, so that we actually go and fix those things? 😛

These observations aside, it looks good to me 👍

JP-Ellis · 2019-05-05T01:32:48Z

Regarding cargo fmt, I completely agree that if formatting fails the build should fail. I'm happy to wait for Add Code Coverage #628 to be merged first and then this pull request will add code coverage.
I forgot the mention in the original post, but I have also added the flags -D warnings and -D missing_docs which causes compilation to fail if there are any warnings or undocumented (public) functions/structs/traits. Since the code coverage is only executed after everything else succeeds, I had to add /// TODO: Document in various places as a temporary fix so that CI can pass and run the code coverage.

If you would like, I'm happy to add a commit reverting these changes just before we merge this.
clippy is set at the moment to not cause the build the fail. This is because there are certain (rare) cases where clippy gets it wrong, but one particularly relevant to ndarray is the use of the slice macro s![...].

LukeMathWalker · 2019-05-12T15:29:41Z

I forgot the mention in the original post, but I have also added the flags -D warnings and -D missing_docs which causes compilation to fail if there are any warnings or undocumented (public) functions/structs/traits. Since the code coverage is only executed after everything else succeeds, I had to add /// TODO: Document in various places as a temporary fix so that CI can pass and run the code coverage.
If you would like, I'm happy to add a commit reverting these changes just before we merge this.

I'd prefer to see those reverted for now - we can open another PR to properly fill in the missing gaps and then start failing builds if docs are missing.
I have merged #635, so if you can re-run the formatter we should be ready to go.

JP-Ellis · 2019-05-13T01:00:25Z

The -D warnings flag was easy to fix as there was only one issue (the use of a deprecated feature in one of the examples or benches). So if that's alright, I'll leave that in the pull request. EDIT: Actually, there are a few more issues which need to be fixed to prevent warnings, but these are still small compared to the missing docs.

I'll disable -D missing_docs for now and open up another issue regarding missing_docs.

LukeMathWalker · 2019-06-23T13:08:15Z

I'd like to go ahead and merge this - do you want to have a look first @jturner314 @termoshtt?

jturner314

This is great progress in the right direction, but there are a few issues that need to be resolved before we merge this.

jturner314 · 2019-06-26T00:47:01Z

.travis.yml

 sudo: required
-dist: trusty
+
+rust:


We should keep the fixed Rust version (1.31.0) as one of the tested versions in addition to the latest stable. (This prevents us from accidentally merging code that requires a newer Rust version without realizing it.)

This is going to be a little annoying to do as some of the code checking dependencies require a newer version. Would it be acceptable if code coverage is disable on 1.31.0?

jturner314 · 2019-06-26T01:05:54Z

build.rs


 fn main() {
    println!("cargo:rerun-if-changed=build.rs");
-    if cfg!(feature = "test-blas-openblas-sys") {
+    if cfg!(feature = "blas") {


This isn't quite right. The BLAS backend is user-selectable. We shouldn't emit a linking directive for the OpenBLAS backend unless testing ndarray's BLAS support. (This directive should not be emitted just because BLAS support is enabled, because the user may want to use a different backend.)

I think the cleanest solution is to delete build.rs entirely (and the corresponding line in Cargo.toml), then add the following to Cargo.toml:

[dependencies] ... # This should be used only for the `test-blas-openblas-sys` feature. openblas-src = { version = "0.6.0", optional = true, default-features = false } [features] ... # This feature is used for testing BLAS support with the system OpenBLAS. test-blas-openblas-sys = ["blas", "blas-src/openblas", "openblas-src", "openblas-src/cblas", "openblas-src/system"]

Then, to test BLAS support using the system's installed OpenBLAS, enable the test-blas-openblas-sys feature. (I've tried running cargo test --features=test-blas-openblas-sys on my computer, and this does work without the "openblas-src/system" feature, but not with that feature. I suspect that this is just an issue with my computer's OpenBLAS installation, and that it'll work fine on CI.)

Ok, I was misunderstanding how this works (and I'm not sure I fully understand what you would like). My reasoning was that if seems weird to have a feature called test-foo. If anything, should we not have a foo feature and then run these tests of cfg!(test) && cfg!(feature = "foo")?

jturner314 · 2019-06-26T01:44:19Z

.travis.yml

+rust:
+  - stable
+  - beta
+  - nightly


The "nightly" feature needs to be enabled for the nightly run (so that e.g. the benchmarks get compiled).

Yes, that is true. I thought I was doing that, but I'll double check.

jturner314 · 2019-06-26T01:50:44Z

parallel/Cargo.toml

-rayon = { version = "1.0" }
-ndarray = { version = "0.12.0", path = "../" }
+rayon = "1.0"
+ndarray = { version = "0.12.0", path = "../", features = ["rayon"]}


I'm curious; why is the "rayon" feature necessary here?

Note that the ndarray-parallel crate (the stuff in the parallel directory) became obsolete after its functionality was added directly to ndarray behind the "rayon" feature.

If I remember correctly, it wasn't running the tests correctly without the rayon feature being enabled (but it was a little while ago that I did this).

jturner314 · 2019-06-26T01:58:04Z

ci/after_success.sh

+    echo "Uploaded code coverage to codecov.io"
+}
+
+coverage_coveralls() {


I'd prefer to remove coveralls and just use codecov.io (since it would be nice to pick just one for simplicity, and codecov.io is already used for ndarray-stats). IIUC, this would eliminate the need for the COVERAGE_RUN variable.

I had both as it isn't anymore complicated, but I can certainly just have one.

jturner314 · 2019-06-26T02:02:43Z

ci/after_success.sh

+    # Run kcov on all the test suites
+    if [[ $COVERAGE_RUN != "true" ]]; then
+        cargo coverage --all --tests --benches --no-default-features
+        mv target/kcov target/kcov-no-default-features


Do we need to move the coverage results like this? The docs for the cargo coverage command seem to indicate that it automatically merges all results together into target/kcov when run multiple times, but I may be misinterpreting the docs.

Unfortunately yes. cargo coverage merges results from one run, but not from several different runs with different features unfortunately.

jturner314 · 2019-06-26T02:03:36Z

ci/after_success.sh

+        cargo coverage --all --tests --benches --no-default-features
+        mv target/kcov target/kcov-no-default-features
+        cargo coverage --all --tests --benches --features "$FEATURES"
+        mv target/kcov target/kcov-features


The commands above don't run all the tests. (They miss the ones in serialization-tests, blas-tests, and numeric-tests.)

That's odd; the --all flag should run all tests from all workspaces. Could this be left as-is and once this pull request in merged, I'll work on #637?

numeric-tests is explicitly not in the workspace, so that it can use its own profile/compilation settings. It can be added if that can be achieved another way

I don't see really see anything special about numeric-tests which can't be included as part of #637. If it is because of a specific combination of features, then we can easily add them by adding to the Travis-CI test matrix. If there's something more subtle, perhaps this can be addressed in #637?

jturner314 · 2019-06-26T02:08:21Z

ci/script.sh

+# Run the test suite.
+check_tests() {
+    cargo test --all --examples --tests --benches --no-default-features
+    cargo test --all --examples --tests --benches --features "$FEATURES"


IIUC, the commands above don't run all the tests. (They miss the ones in serialization-tests, blas-tests, and numeric-tests.)

As I see it, we have two options:

Keep the existing serialization-tests, blas-tests, and numeric-tests crates and run their tests in the scripts.

Merge them into the ndarray crate, adding the necessary optional dependencies and creating the necessary test-specific features in ndarray, similar to how I'm suggesting to handle the test-blas-openblas-src feature.

I think option 2 is a bit cleaner, but either is fine with me. Either way, we need to make sure that we run these tests.

I created an issue about specifically this in #637. I agree that keeping these scripts separate isn't nice and integrating them I think would be the best option.

max-sixty · 2019-07-18T15:04:02Z

@JP-Ellis I'm happy to help on integrating the test workspaces, if you can find some minimal version of this to merge in

- This pull requests also runs `cargo clippy`, though warnings raised by clippy *do not* cause the build to fail. - Also added the flag `nightly` for features only available on nightly (such as the `test` crate for benching). Simultaneously remove the `test` feature as this is automatically handled by `cargo test`, and removed the `test-blas-openblas-sys` as it is equivalent to having `cargo test --features blas`. Signed-off-by: JP-Ellis <[email protected]>

One warning is due to the use of the deprecated `RcArray`, the other is due to the new `approx` change in master.

Signed-off-by: JP-Ellis <[email protected]>

JP-Ellis · 2019-09-03T07:15:46Z

I've rebased this pull request on top of master. I've also incorporated some of the changes mentioned above, including:

Added a missing nightly features on the nightly build
Only build cargo-travis on stable, and only run code coverage on stable.
Remove part of the code relating to coveralls.

Let me know if there's anything else that should be added?

There's one thing discussed above which I haven't changed: the test-blas-openblas-sys feature as I believe (though may be incorrect) that it can be replaced by an appropriate combination of cfg!(test) && cfg!(feature = "openblas") (or whatever feature is relevant).

Note that I've also switch from using cargo-travis to cargo-kcov as it seems a lot simpler to use.

It might be worth considering cargo-tarpaulin as an alternative, though at this stage I hope to get this pull request merged.

Signed-off-by: JP-Ellis <[email protected]>

LukeMathWalker mentioned this pull request May 4, 2019

Run rustfmt and add to travis #608

Closed

LukeMathWalker mentioned this pull request May 10, 2019

Rust cargo-fmt #635

Merged

max-sixty mentioned this pull request Jun 24, 2019

Add code coverage #641

Closed

jturner314 requested changes Jun 26, 2019

View reviewed changes

LukeMathWalker mentioned this pull request Jun 30, 2019

Travis runs on all branches #645

Merged

LukeMathWalker mentioned this pull request Jul 18, 2019

Integrate Test Workspaces/Crates #637

Closed

JP-Ellis added 4 commits September 3, 2019 15:29

Fix 2 sets of warnings:

dee845a

One warning is due to the use of the deprecated `RcArray`, the other is due to the new `approx` change in master.

Remove coveralls support in favour of only codecov

deddcd9

Signed-off-by: JP-Ellis <[email protected]>

Only install cargo-travis on stable

23717be

Signed-off-by: JP-Ellis <[email protected]>

JP-Ellis added 4 commits September 3, 2019 17:19

Add "nightly" features

13cdbbc

Signed-off-by: JP-Ellis <[email protected]>

Switch from cargo-travis to cargo-kcov

4a1fd42

Signed-off-by: JP-Ellis <[email protected]>

No need to explicitly specify --examples, --tests, --benches

2f363e9

Signed-off-by: JP-Ellis <[email protected]>

Have to install build kcov as repository kcov is too old

5379aa1

Signed-off-by: JP-Ellis <[email protected]>

JP-Ellis closed this Nov 20, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Code Coverage #628

Add Code Coverage #628

JP-Ellis commented May 4, 2019 •

edited

Loading

codecov-io commented May 4, 2019 •

edited by codecov bot

Loading

LukeMathWalker commented May 4, 2019 •

edited

Loading

JP-Ellis commented May 5, 2019

LukeMathWalker commented May 12, 2019

JP-Ellis commented May 13, 2019 •

edited

Loading

LukeMathWalker commented Jun 23, 2019

jturner314 left a comment

jturner314 Jun 26, 2019

JP-Ellis Sep 3, 2019

jturner314 Jun 26, 2019

JP-Ellis Sep 3, 2019

jturner314 Jun 26, 2019

JP-Ellis Sep 3, 2019

jturner314 Jun 26, 2019

JP-Ellis Sep 3, 2019

jturner314 Jun 26, 2019

JP-Ellis Sep 3, 2019

jturner314 Jun 26, 2019

JP-Ellis Sep 3, 2019

jturner314 Jun 26, 2019

JP-Ellis Sep 3, 2019

bluss Sep 4, 2019

JP-Ellis Sep 4, 2019

jturner314 Jun 26, 2019

JP-Ellis Sep 3, 2019

max-sixty commented Jul 18, 2019 •

edited

Loading

JP-Ellis commented Sep 3, 2019 •

edited

Loading

Add Code Coverage #628

Add Code Coverage #628

Conversation

JP-Ellis commented May 4, 2019 • edited Loading

codecov-io commented May 4, 2019 • edited by codecov bot Loading

Codecov Report

LukeMathWalker commented May 4, 2019 • edited Loading

JP-Ellis commented May 5, 2019

LukeMathWalker commented May 12, 2019

JP-Ellis commented May 13, 2019 • edited Loading

LukeMathWalker commented Jun 23, 2019

jturner314 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

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

max-sixty commented Jul 18, 2019 • edited Loading

JP-Ellis commented Sep 3, 2019 • edited Loading

JP-Ellis commented May 4, 2019 •

edited

Loading

codecov-io commented May 4, 2019 •

edited by codecov bot

Loading

LukeMathWalker commented May 4, 2019 •

edited

Loading

JP-Ellis commented May 13, 2019 •

edited

Loading

max-sixty commented Jul 18, 2019 •

edited

Loading

JP-Ellis commented Sep 3, 2019 •

edited

Loading