UINT32 mods in support of schema-printer PR #342 #343
Conversation
|
This pull request has been linked to Shortcut Story #12282: Expand R-UDF validation coverage: make sure essential API calls work. |
johnkerl
force-pushed
the
kerl/sc-12282-uint32
branch
from
1530b81 to
a777faf
Compare
johnkerl
force-pushed
the
kerl/sc-12282-uint32
branch
from
a777faf to
c3dbb65
Compare
inst/tinytest/test_libtiledb.R
Outdated
| @@ -131,6 +131,15 @@ attr <- tiledb:::libtiledb_attribute(ctx, "a1", "INT32", filter_list, 1, FALSE) | |||
| expect_true(is(attr, "externalptr")) | |||
| #}) | |||
|
|
|||
| #test_that("basic integer libtiledb_attr constructor works", { | |||
| ##ctx <- tiledb:::libtiledb_ctx() | |||
| ctx <- tiledb_get_context()@ptr | |||
There was a problem hiding this comment.
(That's an old test file and an old / unusual construct. On the margin I'd rather not propagate it.)
inst/tinytest/test_libtiledb.R
Outdated
| filter <- tiledb:::libtiledb_filter(ctx, "NONE") | ||
| filter_list <- tiledb:::libtiledb_filter_list(ctx, c(filter)) | ||
| attr <- tiledb:::libtiledb_attribute(ctx, "a1", "UINT32", filter_list, 1, FALSE) | ||
| expect_true(is(attr, "externalptr")) |
There was a problem hiding this comment.
That is sadly a somewhat uninformative test (as 'anything' could be an external pointer). Not much we can do about it. You could test that the attr is in fact an attribute but maybe this whole block is marginal.
There was a problem hiding this comment.
Thanks @eddelbuettel ! I made an effort to copy UINT32 attribute-fill-value tests from all the existing INT32 attribute-fill-value test I could find -- perhaps we can improve both the old and the new on this PR? Or on a separate PR perhaps?
There was a problem hiding this comment.
Let's just remnove this block. We have a test for UINT32 which is good.
There is an testfile inst/tinytest/test_filterlist.R so if the test is for filterlist changes maybe that would have been better?
There was a problem hiding this comment.
Thanks @eddelbuettel ! I'll take a look. Perhaps that will be a good place to add coverage for old INT32 cases as well as new UINT32 coverage.
There was a problem hiding this comment.
@eddelbuettel on further reflection, I believe lines 89-101 of inst/tinytest/test_attr.R on this PR are the right location, since they test tiledb_attribute_set_fill_value and tiledb_attribute_get_fill_value which are functions modified on this PR.
There was a problem hiding this comment.
Absolutely!
I was harping about the changes to a file I could have / should have nuked but didn't and that change you kindly removed. This one is now tight and focussed..
|
Replied on #343 (comment) -- please let me know what you think -- thanks @eddelbuettel ! |
|
Thanks @eddelbuettel ! |
This is in support of #342, which will be rebased atop this.