[datatable-help] Streamlining unit tests with test_that

Thu Dec 23 22:59:14 CET 2010

Hi Steve,

Sorry not had time to look at this yet. Hoping to soon. Let me know if
you're held up. It's high priority my side too as I'd like to fix
the .Depends issue, the 2 outstanding bugs, get cedta sorted and release
those few things onto CRAN soon. S4 tests would be nice to include.

One thought though .. how would we know the ported tests correctly fail
when they should?  Or perhaps it's easier to ask ... how would we know
the ported tests fail in the same conditions the old tests fail?

So I'm thinking we should keep the old test.data.table() there running
even if it's just locked down. We could put new tests into test_that
going forward (starting with S4) and that would save you the time of
porting. Would that work?

Matthew

On Fri, 2010-12-17 at 22:50 -0500, Steve Lianoglou wrote:
> Hi all,
> 
> After a long delay, I've gotten around to testing some of the
> test_that-ized unit tests and everything seems clean using Tom's
> changes to cedta.R.
> 
> Here are what my S4 tests are like:
> https://github.com/lianos/datatable/blob/testthat/pkg/inst/tests/test-S4.R
> 
> These run clean and don't have the problem with "locked environments"
> and stuff that the "normal tests" have that are being run through a
> man/*.Rd file.
> 
> To give you an idea of what some of the current tests (1-16) look like
> when they are test_that-ized, look here:
> https://github.com/lianos/datatable/blob/testthat/pkg/inst/tests/test-all.R
> 
> They can be even simpler to write if you don't insist on keeping track
> of their test IDs (numbers). For instance, test 1 can go from this:
> 
>   expect_that(TESTDT[SJ(4,6),v,mult="first"], is_identical_to(3L), info="1")
> 
> to this:
> 
>   expect_identical(TESTDT[SJ(4,6),v,mult="first"], 3L)
> 
> Running tests are very simple as well. You can run them in "the usual
> way" via R CMD check. When you do so, the results look like so:
> 
> $ R CMD check <data.table>
> ...
> ... R CMD check progress ...
> ** running tests for arch 'i386'
>   Running ?test-all.R?
>  OK
> ** running tests for arch 'x86_64'
>   Running ?test-all.R?
>  OK
> ...
> 
> I deliberately put in a wrong expected result to show you what it
> looks like when a test like this fails:
> 
> $ R CMD check <data.table>
> ...
> ... R CMD check progress ...
> ** running tests for arch 'i386'
>   Running ?test-all.R?
>  ERROR
> Running the tests in 'tests/test-all.R' failed.
> Last 13 lines of output:
> 
>   S4-isms : .....
>   Sorted Joins : 1...............
> 
>   1. Failure: basic sorted joins work
> --------------------------------------------
>   TESTDT[SJ(4, 6), v, mult = "first"] is not identical to 4L. Differences:
>   Mean relative difference: 0.25
>   1
>   Error: Test failures
>   In addition: Warning message:
>   In getPackageName(where) :
>     Created a package name, "2010-12-17 22:23:34", when none found
>   Execution halted
> ...
> 
> You can also run the unit tests in your "live" R session like so:
> 
> R> library(data.table)
> R> library(testthat)
> R> test_dir('/path/to/datatable/pkg/inst/tests')
> 
> or a single file at a time:
> 
> R> test_file(''/path/to/datatable/pkg/inst/tests/test-something.R")
> 
> If you'd consider moving the test suite over to test_that, I'll
> volunteer to do the conversions over the next week (or so (maybe after
> xmas)). We can organize the tests a bit more cleanly into different
> files and "contexts" to be a bit more explicit about which groups of
> tests are testing what types of functionality if you like.
> 
> If you don't want to keep track of unit tests by their number, than
> the conversions will be a bit less cumbersome, but it's your choice.
> 
> Regardless of the unit test framework switch, my "S4-ism" code is
> ready to land back in trunk as it stands now. The tests for adding
> generic methods to an S4 object that uses the data.table won't work
> with the current test.data.table.R stuff, so I'd just keep those out
> for now. See the commented part of the testing blocks at the bottom
> here to see what I mean (tests 228 to 231):
> 
> https://github.com/lianos/datatable/blob/master/pkg/R/test.data.table.R
> 
> Sorry for the long email, but there are two issues rolled into one here.
> 
> Thanks,
> -steve
> 
> On Wed, Dec 15, 2010 at 6:47 PM, Matthew Dowle <mdowle at mdowle.plus.com> wrote:
> > Hi Tom,
> >
> > I tested your change to cedta - passes build, tests and vignettes. Looks
> > good. It didn't fix bug 1131 (I had hoped it might too) but I fixed that
> > by adding a check on whether the caller is the debugger frame.
> >
> > Could you commit your change please then I'll do mine on top.
> >
> > Matthew
> >
> >
> > On Mon, 2010-12-06 at 22:37 -0500, Steve Lianoglou wrote:
> >> Hi,
> >>
> >> On Mon, Dec 6, 2010 at 9:17 PM, Matthew Dowle <mdowle at mdowle.plus.com> wrote:
> >> > Been offline for a few days, just catching up with these threads.
> >> >
> >> > Yes - Tom's idea looks good to me. .GlobalEnv would be considered
> >> > data.table aware since it isn't a namespace then, thus short-circuiting
> >> > the first || when called from the user workspace and retaining the
> >> > efficiency fix of the change from cendta to cedta a while back (that the
> >> > identical(te,.GlobalEnv) was there for) so I'm happy.
> >> >
> >> > The topenv when the caller is a namespaceless package is .GlobalEnv
> >> > rather than the package (if I remember the issue correctly) which is the
> >> > reason namespaceless packages are incorrectly considered data.table
> >> > aware. As Tom says that's an issue anyway and shouldn't be affected by
> >> > this change. In practice it's not an issue since we're not aware of any
> >> > namespaceless packages being used by data.table users. Have changed the
> >> > priority of that tracker item to low (which oddly on R-Forge is 1 not
> >> > 5).
> >> >
> >> > The new cedta might well fix this bug neatly too :
> >> > https://r-forge.r-project.org/tracker/index.php?func=detail&aid=1131&group_id=240&atid=975
> >> >
> >> > Haven't looked at the test_that proposal yet. Shall I look once these
> >> > issues are resolved and a few tests are working ok?
> >>
> >> Yeah, I've been meaning to implement Tom's suggestion in my testthat
> >> branch and send you guys a link to checkout, but I've been busy trying
> >> to catch up w/ stuff at school.
> >>
> >> I'll work on something for you to look at and send you an update in
> >> the next day or two.
> >>
> >> -steve
> >>
> >
> >
> >
> 
> 
> 