Skip to content

Increase coverage beyond 80% - #1

Merged
osenan merged 7 commits into
masterfrom
add_test@master
Jun 16, 2026
Merged

Increase coverage beyond 80%#1
osenan merged 7 commits into
masterfrom
add_test@master

Conversation

@osenan

@osenan osenan commented May 29, 2026

Copy link
Copy Markdown

This PR adds unit tests for all the functions exported in the NAMESPACE.

The package does not have CI and is not using roxygen2 comments to manage documentation and function exports.

You have to run the tests locally and coverage as well:


devtools::test()

covr::package_coverage()

This is my output:

image

The only tricky test was to find a table with the distribution of the Anderson-Darling statistic, but I finally used a reference mentioned in the paper that this package is based on.

@osenan
osenan requested a review from a team May 29, 2026 11:02
@osenan osenan changed the title Add test@master Increase coverage beyond 80% May 29, 2026
@llrs-roche

Copy link
Copy Markdown

Now the work is (nicely) done, specially the tables to test the corner cases 🚀

But this adds a new dependency the maintainer might not be willing to accept. I would check with them if they are willing to depend on it, as it seems they didn't use in the past. This is a new burden for them that they will need to maintain the package and the tests. For example currently the suggested package testthat is not used conditionally. If the package were missing or failed to install correctly the gofest package would fail tests (and yes, this happened on CRAN)

@averissimo averissimo self-assigned this Jun 2, 2026

@averissimo averissimo left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's at 90.42% and the missing ones are probably unreachable as they are C functions not being used in R code.

I have some minor comments and mirroring @llrs-roche concern about testthat, the tests are possible without adding that dependency.

Can you adjust some of the value-based tests from SciPy?

Image

Comment thread tests/testthat/test-andarl.R
Comment thread tests/testthat/test-recog.R
Comment thread tests/testthat/test-cvmtest.R Outdated
Comment on lines +67 to +75
it("returns a valid qCvM quantile", {
q <- qCvM(0.9)
expect_true(is.numeric(q) && q > 0)
expect_equal(pCvM(q), 0.9, tolerance = 1e-3)
})

it("returns matching qCvM quantiles for lower.tail inversion", {
expect_equal(qCvM(0.1, lower.tail = FALSE), qCvM(0.9))
})

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seem out of place of the describe block, maybe move out on itself

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True, changed

Comment thread tests/testthat/test-cvmtest.R
@averissimo

Copy link
Copy Markdown

Consider adding CI actions for R CMD check with usethis::use_github_action("check-full")

This might require some discussion with package owner, but it's such a small quality of life improvement, that it shouldn't be a problem.

Or just add it here and we'll remove it on PR against upstream

ps: going the extra mile with some concurrency protection.

concurrency:
  group: ${{ github.workflow }}-${{ github.ref }}
  cancel-in-progress: ${{ github.ref != 'refs/heads/main' }}

@osenan

osenan commented Jun 8, 2026

Copy link
Copy Markdown
Author

Hi, thanks for the suggestion.
Beyond specific comments, I have added withr to Suggests. I have not added the CI for the moment. Once approved I send the PR to the package authors and will ask if we can add CI checks. If not, I will add them to this repo anyway.

@averissimo averissimo left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good 🎩

I left some more tests (these were generated with AI converting the rest of SciPy tests)

I also noticed the CMD check (--as-cran) warns about versioning. It seems that the released version is superior to the git repo. However, the code (R and C) is identical, it only has some folders that don't seem necessary.

Comment thread DESCRIPTION Outdated
Comment thread DESCRIPTION Outdated
Comment thread tests/testthat/test-pCvM.R Outdated
osenan and others added 3 commits June 16, 2026 07:45
Co-authored-by: André Veríssimo <211358+averissimo@users.noreply.github.com>
Co-authored-by: André Veríssimo <211358+averissimo@users.noreply.github.com>
Co-authored-by: André Veríssimo <211358+averissimo@users.noreply.github.com>
@osenan
osenan merged commit b521e11 into master Jun 16, 2026
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.

3 participants