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

custom expectation for testthat #74

Open
kieranjmartin opened this issue Mar 9, 2021 · 3 comments
Open

custom expectation for testthat #74

kieranjmartin opened this issue Mar 9, 2021 · 3 comments
Assignees
Labels
enhancement New feature or request
Milestone

Comments

@kieranjmartin
Copy link
Collaborator

A nice integration might be to have something like expect_equal_df for testthat. This could generate nice error messages for the testing reportt

@gowerc
Copy link
Owner

gowerc commented Mar 15, 2021

this is actually a neat idea....

My only concern would then be though that it requires adding test that to the dependencies which is super not light

image

I guess we could include it as a suggests which could limit that somewhat ... ?

@thomas-neitmann
Copy link

After raising this to @kieranjmartin I actually implemented this for one of my packages recently. The code is fairly simple.

#' Expectation: Are Two Datasets Equal
#'
#' Uses [diffdf::diffdf()] to compares 2 datasets for any differences
#'
#' @param base Input dataset
#' @param compare Comparison dataset
#' @param keys `character` vector of variables that define a unique row in the
#'        base and compare datasets
#' @param ... Additional arguments passed onto [diffdf::diffdf()]
#'
#' @examples
#' testthat::test_that("a missing row is detected", {
#'   data(dm)
#'   expect_dfs_equal(dm, dm[-1L, ], keys = "USUBJID")
#' })
#'
expect_dfs_equal <- function(base, compare, keys, ...) {
  diff <- diffdf::diffdf(base, compare, keys, suppress_warnings = TRUE, ...)
  if (diffdf::diffdf_has_issues(diff)) {
    msg <- capture.output(print(diff))
    testthat::fail(msg)
  } else {
    testthat::succeed()
    invisible()
  }
}

I'd be happy to open a pull request for this.

Regarding the {testthat} dependency I would indeed add it to Suggests rather than Imports.

@gowerc
Copy link
Owner

gowerc commented Apr 1, 2021

I was thinking about this the other day a bit more and I can't make up my mind on it.

my main concern is does it provide enough value to be worth including testthat as a dependency. I have a strong preference to keep the dependency list as small as possible. Realistically I'm not sure I'm convinced this then adds enough utility. Like the test is already possible with exepect_equal() the only difference is the error message which I would expect users to manually use to diagnose.

Though having said that I guess this is a nice way of getting diffdf to throw an error without suppressing the comparison information. hmmmm @kieranjmartin any thoughts ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants