Cast to data.frame to ignore other data.frame-extended classes in get_query_good_after_bad_n

[MichaelChirico]

Related: #400.

From test_result(run_only = 'get_query_good_after_bad_n') I am getting:

── Failure: DBItest: Result: get_query_good_after_bad_n ─────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────
`rows` (`actual`) not equal to data.frame(a = 1.5) (`expected`).

`class(actual)`:   "tbl_df" "tbl" "data.frame"
`class(expected)`:                "data.frame"
Backtrace:
    ▆
 1. └─DBItest:::spec_result$get_query_good_after_bad_n(con = global_con)
 2.   └─testthat::expect_equal(rows, data.frame(a = 1.5)) at rbuild/R/spec-result-get-query.R:83:5

Error:
! Test failed
Run `rlang::last_trace()` to see where the error occurred.

Another option is to weaken the expected value side:

expect_length(rows, 1L)
expect_identical(rows[["a"]], 1.5)

(check_df() just above already handles the data.frame check)

[MichaelChirico]

Actually there are a number of such places, I will make the change once I get some feedback that this is likely to be accepted. Thanks!

[krlmlr]

Could the "tibble return" be an option to your dbConnect() method that is off when running our tests?

[MichaelChirico]

I think it's possible, the implication being {DBI} expects downstreams to be strictly data.frame-compliant. Two downsides:

1. Maintenance overhead of having to have some extra cruft around that's putatively only for testing
2. Of course once such an option shows up in the interface, people will inevitably use it 😄 potentially adding to the maintenance overhead

[krlmlr]

Again, perhaps related to tidyverse/tibble#1570 -- should tibble be more data-frame-compliant, in particular when requested?

[krlmlr]

Can this be a tweak, perhaps?

[MichaelChirico]

Can this be a tweak, perhaps?

Perhaps, though the API isn't immediately obvious to me, would have to think more carefully.

One reaction, though, is that in practice it would be v. similar to the current PR, just more opaque, right?

-expect_equal(rows, data.frame(a = 1.5))
+expect_equal(as.data.frame(rows), data.frame(a = 1.5))

vs.

-expect_equal(rows, data.frame(a = 1.5))
+expect_equal(context$tweaks$standardize_return(rows), data.frame(a = 1.5))

Something like that?

[krlmlr]

Oh -- so check_df() already allows data frame subclasses? Introduced in 31e86cd, or even before that?

From a "standards" perspective, it's easier to specify that the returned class must be a data frame and not a subclass. We could also come up with a specification of behavior that works for both data frames and tibbles.

I'd really like to turn DBItest into a code generator that emits tests into the backend packages. We could make this (potentially breaking?) change as part of that exercise.