[MXNET-851] Test coverage metrics for R-package #12391

anirudhacharya · 2018-08-29T06:51:36Z

Description

Track test coverage metrics for R-package using covr package.

Checklist

Essentials

Please feel free to remove inapplicable items for your PR.

The PR title starts with [MXNET-$JIRA_ID], where $JIRA_ID refers to the relevant JIRA issue created (except PRs with tiny changes)
Changes are complete (i.e. I finished coding on this PR)
All changes have test coverage:
Unit tests are added for small changes to verify correctness (e.g. adding a new operator)
Nightly tests are added for complicated/long-running ones (e.g. changing distributed kvstore)
Build tests will be added for build configuration changes (e.g. adding a new build option with NCCL)
Code is well-documented:
For user-facing API changes, API doc string has been updated.
For new C++ functions in header files, their functionalities and arguments are documented.
For new examples, README.md is added to explain the what the example does, the source of the dataset, expected performance on test set and reference to the original paper if applicable
Check the API doc at http://mxnet-ci-doc.s3-accelerate.dualstack.amazonaws.com/PR-$PR_ID/$BUILD_ID/index.html
To the my best knowledge, examples are either not affected by this change, or have been fixed to be compatible with this change

Comments

@marcoabreu

marcoabreu · 2018-08-29T08:52:50Z

Thank you! Could you elaborate how and when this script is getting executed? I guess we have to modify our test pipeline and add it there

marcoabreu · 2018-08-29T09:14:48Z

https://github.com/apache/incubator-mxnet/blob/master/Makefile#L591

marcoabreu · 2018-08-29T09:15:35Z

https://github.com/apache/incubator-mxnet/blob/master/ci/docker/runtime_functions.sh#L738

marcoabreu · 2018-08-29T23:33:40Z

Thanks. Did you enable the proper error propagation?

anirudhacharya · 2018-08-30T00:20:23Z

@marcoabreu the error propagation issue is due to a bug in the test framework that we are using, testthat. I am trying to find a workaround for that

marcoabreu · 2018-08-30T00:30:45Z

Great, thank you.

Do you want to fix that in a separate PR?

anirudhacharya · 2018-08-30T03:22:56Z

yes, I will make that another PR.

marcoabreu · 2018-08-30T09:25:56Z

R-package/ci-test-coverage.sh

+
+MXNET_HOME=${PWD}
+cd ${MXNET_HOME}/R-package
+Rscript -e 'covr::codecov()'


I think this script is obsolete now

anirudhacharya · 2018-08-31T06:17:49Z

test coverage report - http://jenkins.mxnet-ci.amazon-ml.com/blue/rest/organizations/jenkins/pipelines/incubator-mxnet/branches/PR-12391/runs/6/nodes/133/steps/154/log/?start=0

@marcoabreu The CI run generates the report but it is not able to upload the reports to codecov due to the following error -

Please provide the repository token to upload reports via `-t :repository-token`

marcoabreu · 2018-08-31T09:39:41Z

Do you see the publish_test_coverage call in our Jenkinsfile? It's important that this call submits the report to codecov. Here, it seems like the package is trying to upload the report itself. Is it possible to just output the test coverage report into the workdir? Codecov will then automatically pick it up

anirudhacharya · 2018-08-31T16:23:55Z

@marcoabreu this run - http://jenkins.mxnet-ci.amazon-ml.com/blue/organizations/jenkins/incubator-mxnet/detail/PR-12391/6/pipeline/133 just outputs the coverage report to the console, but codecov is unable to retrieve/parse the results.

marcoabreu · 2018-08-31T20:06:32Z

Have a look at the source here: https://github.com/r-lib/covr/blob/master/R/codecov.R

They automatically try to upload to codecov. We don't want that. Instead, we want this output to be redirected to a file. Our method then automatically picks that file up and uploads it properly.

Basically what I would recommend is to take that function and replicate it to redirect the output to a file. Can you try that?

The reason we don't want this implicit upload is because of high availability. We took some measures because codecov is not really reliable in terms of availability of their default endpoints.

anirudhacharya · 2018-09-11T16:08:35Z

@marcoabreu please review.

marcoabreu · 2018-09-11T16:24:43Z

Thanks a lot!

It seems like the files are not showing up as covered here though :(

https://codecov.io/gh/apache/incubator-mxnet/tree/3e6bae80d0dd975d3865dc59727aa3a4b2f939c7

anirudhacharya · 2018-09-11T16:26:07Z

but as you suggested, i am writing it to a json file in the local directory.

marcoabreu · 2018-09-11T16:26:38Z

Can you upload the content of that file on pastebin or gist? I'd love to have a closer look to investigate

anirudhacharya · 2018-09-11T16:31:53Z

an example test coverage json - https://drive.google.com/file/d/1xlynn-rBtpEPVl6De64fB76qjeO-Nh__/view?usp=sharing

marcoabreu · 2018-09-11T16:32:19Z

Makefile

 rpkgtest:
-	Rscript -e "require(testthat);res<-test_dir('R-package/tests/testthat');if(!testthat:::all_passed(res)){stop('Test failures', call. = FALSE)}"
+	Rscript -e 'require(testthat);res<-test_dir("R-package/tests/testthat");if(!testthat:::all_passed(res)){stop("Test failures", call. = FALSE)}'
+	Rscript -e 'res<-covr:::package_coverage("R-package");fileConn<-file("r-package_coverage.json");writeLines(RJSONIO:::toJSON(res), fileConn);close(fileConn)'


I think this functions output does not really match the format codecov expects. Have a look at this line:

https://github.com/r-lib/covr/blob/master/R/codecov.R#L164

marcoabreu

It's working!

https://codecov.io/gh/apache/incubator-mxnet/tree/c9935b1ace60e10086f4f507355f37898a0f4288/R-package/src

marcoabreu · 2018-09-13T14:42:32Z

Thanks a lot! It seems like codecov is having trouble associating the proper commit hash of a PR with the report, but that's a separate problem. This PR has successfully added test coverage for R, great job!

anirudhacharya · 2018-09-13T17:33:34Z

@marcoabreu Thanks! Though the current coverage report is not completely accurate, it shows 0% coverage for certain files which have unit tests for them. Will check why covr misses the coverage in those files.

* get test coverage metrics for r-package * update makefile to include test coverage, and fix test failures * write to json file

anirudhacharya requested a review from thirdwing as a code owner August 29, 2018 06:51

anirudhacharya requested a review from szha as a code owner August 29, 2018 22:49

anirudhacharya force-pushed the testCov branch from ca993e1 to 84fdfa8 Compare August 30, 2018 03:22

marcoabreu reviewed Aug 30, 2018

View reviewed changes

anirudhacharya force-pushed the testCov branch 5 times, most recently from 828e35b to 37578be Compare August 31, 2018 05:53

anirudhacharya force-pushed the testCov branch 2 times, most recently from f3b2a06 to 6985af6 Compare September 11, 2018 04:33

Anirudh Acharya added 2 commits September 10, 2018 22:29

get test coverage metrics for r-package

c972b92

update makefile to include test coverage, and fix test failures

2f1f5d4

anirudhacharya force-pushed the testCov branch from 6985af6 to 3e6bae8 Compare September 11, 2018 05:29

marcoabreu reviewed Sep 11, 2018

View reviewed changes

write to json file

c9935b1

anirudhacharya force-pushed the testCov branch from 3e6bae8 to c9935b1 Compare September 11, 2018 23:59

marcoabreu approved these changes Sep 13, 2018

View reviewed changes

marcoabreu merged commit f387df4 into apache:master Sep 13, 2018

anirudhacharya deleted the testCov branch September 13, 2018 17:26

shadogray mentioned this pull request Oct 27, 2018

R package build broken, Roxygen2 refuses to overwrite NAMESPACE #12993

Closed

[MXNET-851] Test coverage metrics for R-package #12391

[MXNET-851] Test coverage metrics for R-package #12391

Uh oh!

Conversation

anirudhacharya commented Aug 29, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Checklist

Essentials

Comments

Uh oh!

marcoabreu commented Aug 29, 2018

Uh oh!

marcoabreu commented Aug 29, 2018

Uh oh!

marcoabreu commented Aug 29, 2018

Uh oh!

marcoabreu commented Aug 29, 2018

Uh oh!

anirudhacharya commented Aug 30, 2018

Uh oh!

marcoabreu commented Aug 30, 2018

Uh oh!

anirudhacharya commented Aug 30, 2018

Uh oh!

marcoabreu Aug 30, 2018

Choose a reason for hiding this comment

Uh oh!

anirudhacharya commented Aug 31, 2018

Uh oh!

marcoabreu commented Aug 31, 2018

Uh oh!

anirudhacharya commented Aug 31, 2018

Uh oh!

marcoabreu commented Aug 31, 2018

Uh oh!

anirudhacharya commented Sep 11, 2018

Uh oh!

marcoabreu commented Sep 11, 2018

Uh oh!

anirudhacharya commented Sep 11, 2018

Uh oh!

marcoabreu commented Sep 11, 2018

Uh oh!

anirudhacharya commented Sep 11, 2018

Uh oh!

marcoabreu Sep 11, 2018

Choose a reason for hiding this comment

Uh oh!

marcoabreu left a comment

Choose a reason for hiding this comment

Uh oh!

marcoabreu commented Sep 13, 2018

Uh oh!

anirudhacharya commented Sep 13, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

anirudhacharya commented Aug 29, 2018 •

edited

Loading