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

Workflow for diagnosing and fixing test failures #165

Closed
hadley opened this issue Apr 4, 2018 · 11 comments
Closed

Workflow for diagnosing and fixing test failures #165

hadley opened this issue Apr 4, 2018 · 11 comments
Projects

Comments

@hadley
Copy link
Member

hadley commented Apr 4, 2018

I think DBItest needs a write up (and some improved tooling) that describes what you should do when a DBItest fails - i.e. how to do you go from a failing test to a minimal reprex that you can rapidly iterate with to fix the problem.

One first step would be to expose an only argument which would be the inverse of skip. Then you could (e.g.) call DBItest::test_driver(only = "data_type_driver") in the console.

@krlmlr
Copy link
Member

krlmlr commented Apr 4, 2018

We already have DBItest::test_some().

I think what we really need is better meta-programming: code that rewrites tests in a form like

con <- dbConnect()
dbDoSomething(con, ...)
dbDisconnect(con)

that is easy to run from a dedicated script. (Does that make sense?)

@hadley
Copy link
Member Author

hadley commented Apr 4, 2018

I don't think DBItest needs more complexity; it needs less. The place to start would be a written guide that (e.g.) discusses test_some()

@krlmlr
Copy link
Member

krlmlr commented Apr 4, 2018

There is a vignette, but it's not comprehensive: http://dbitest.r-dbi.org/articles/test.html.

I see conflicting goals:

  1. align the tests with the written specification
  2. don't repeat yourself
  3. understand the reason of a test failure

DBItest is currently biased to support 1. and 2. well, but not so much 3.. Other options are:

  • 1 and 3: simple tests, still aligned with the specs, but created by copy+paste
  • 2 and 3: inline roxygen2 comments pose certain restrictions on how to write the tests, if we lift that the code becomes easier to parse

Meta-programming can solve all three goals at once: the spec describes a code generator that creates many simple tests, these tests are then run. If roxygen2 supported "true" literate programming (documentation chunks can appear in arbitrary order in the source files), we could refactor many tests.

@krlmlr
Copy link
Member

krlmlr commented Apr 25, 2018

I agree that debugging test failures is currently very hard and not pleasant. There's no easy way to set a breakpoint in the test suite (other than editing the code and recompiling), and it's hard to trace a particular failure to the source of the problem.

Expanding on the idea of generating many simple tests: DBItest could generate test files (in tests/testthat or just tests), much the same way as roxygen2 generates .Rd files. Then, the workflow would be:

  1. Run DBItest::generate_tests() on the package.
  2. Run devtools::test() against code generated by DBItest.
  3. If failures occur, the generated code is simple enough to step through manually. The DebugReporter can be used to halt execution at the first failure.

@krlmlr
Copy link
Member

krlmlr commented Feb 11, 2019

rlang's call traces may help. Other than that, I think a good short-term solution would be to record all DBI interface calls using a proxy, and logging them (in the form of runnable R code). Using deparse() first, datapasta can help make this code easier to read.

@hadley
Copy link
Member Author

hadley commented Feb 12, 2019

Again, your proposed solution make things more complex. I think that is the wrong direction to be travelling in.

@krlmlr
Copy link
Member

krlmlr commented Feb 24, 2019

As discussed, working on a proof of concept. Capturing calls works so far:

library(rlang)

print_call <- function(...) {
  args <- list(...)

  call <- eval_tidy(quo(call("print_call", !!!args)))
  print(styler::style_text(deparse(call, width.cutoff = 80)))
  invisible(call)
}

print_call()
#> print_call()
print_call(1, 2:3, a = 4:6 + 0, Sys.Date(), b = Sys.time())
#> print_call(1, 2:3, a = c(4, 5, 6), structure(17951, class = "Date"), b = structure(1551033819.57973, class = c(
#>   "POSIXct",
#>   "POSIXt"
#> )))
call <- print_call(iris[1:3, ])
#> print_call(structure(list(Sepal.Length = c(5.1, 4.9, 4.7), Sepal.Width = c(
#>   3.5, 3,
#>   3.2
#> ), Petal.Length = c(1.4, 1.4, 1.3), Petal.Width = c(0.2, 0.2, 0.2), Species = structure(c(
#>   1L,
#>   1L, 1L
#> ), .Label = c("setosa", "versicolor", "virginica"), class = "factor")), row.names = c(
#>   NA,
#>   3L
#> ), class = "data.frame"))
call
#> print_call(list(Sepal.Length = c(5.1, 4.9, 4.7), Sepal.Width = c(3.5, 
#> 3, 3.2), Petal.Length = c(1.4, 1.4, 1.3), Petal.Width = c(0.2, 
#> 0.2, 0.2), Species = c(1L, 1L, 1L)))

Created on 2019-02-24 by the reprex package (v0.2.1.9000)

@krlmlr
Copy link
Member

krlmlr commented Feb 24, 2019

Some tests require two connections to the database, these can be told apart with identical(). Serializations are stable too, but collide:

library(DBI)
con1 <- dbConnect(RSQLite::SQLite())
con2 <- dbConnect(RSQLite::SQLite())
identical(con1, con1)
#> [1] TRUE
identical(con1, con2)
#> [1] FALSE

digest::sha1(serialize(con1, NULL))
#> [1] "9eb8e8e0eb471063f229cf2a05e093a7eac469be"
digest::sha1(serialize(con1, NULL))
#> [1] "9eb8e8e0eb471063f229cf2a05e093a7eac469be"
digest::sha1(serialize(con2, NULL))
#> [1] "9eb8e8e0eb471063f229cf2a05e093a7eac469be"

Created on 2019-02-24 by the reprex package (v0.2.1.9000)

@krlmlr
Copy link
Member

krlmlr commented Feb 24, 2019

Printing calls works for dbConnect() and dbDisconnect().

External pointers can't be deparsed, but this seems to affect only the first argument of each generic. Need to build dictionaries for known connection and result set objects, and assign them to variables.

@krlmlr
Copy link
Member

krlmlr commented Feb 25, 2019

I now have this in a local tweak of RKazam. Proceeding with the implementation in DBItest:

library(RKazam)

drv <- Kazam(RSQLite::SQLite())
#> drv1 <- RSQLite::SQLite()

conn <- dbConnect(drv, file = ":memory:")
#> conn1 <- dbConnect(drv1, file = ":memory:")
dbDisconnect(conn)
#> dbDisconnect(conn1)
#> [1] TRUE

Created on 2019-02-25 by the reprex package (v0.2.1.9000)

@github-actions
Copy link

This old thread has been automatically locked. If you think you have found something related to this, please open a new issue and link to this old issue if necessary.

@github-actions github-actions bot locked and limited conversation to collaborators Dec 15, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
No open projects
krlmlr
  
Awaiting triage
Development

No branches or pull requests

2 participants