I vote to move the tests all over to the new format. I also prefer the expect_identical() syntax. That lets us do away with the test numbers.
- Tom On Thu, Dec 23, 2010 at 4:59 PM, Matthew Dowle <[email protected]> wrote: > 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 <[email protected]> >> 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 <[email protected]> >> >> 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 >> >> >> > >> > >> > >> >> >> > > > _______________________________________________ > datatable-help mailing list > [email protected] > https://lists.r-forge.r-project.org/cgi-bin/mailman/listinfo/datatable-help > _______________________________________________ datatable-help mailing list [email protected] https://lists.r-forge.r-project.org/cgi-bin/mailman/listinfo/datatable-help
