jonkeane commented on a change in pull request #11790:
URL: https://github.com/apache/arrow/pull/11790#discussion_r758496846
##########
File path: r/tests/testthat/test-data-type.R
##########
@@ -395,6 +395,18 @@ test_that("decimal type and validation", {
expect_error(decimal(4, NA), '"scale" must be an integer')
expect_r6_class(decimal(4, 2), "Decimal128Type")
+
+ expect_error(decimal128())
+ expect_error(decimal128("four"), '"precision" must be an integer')
+ expect_error(decimal128(4))
+ expect_error(decimal128(4, "two"), '"scale" must be an integer')
+ expect_error(decimal128(NA, 2), '"precision" must be an integer')
+ expect_error(decimal128(0, 2), "Invalid: Decimal precision out of range: 0")
+ expect_error(decimal128(100, 2), "Invalid: Decimal precision out of range:
100")
Review comment:
If the c++ errors are tested in the c++ code, we don't need to test them
here as well.
##########
File path: r/tests/testthat/test-chunked-array.R
##########
@@ -206,7 +206,7 @@ test_that("ChunkedArray supports empty arrays
(ARROW-13761)", {
int8(), int16(), int32(), int64(), uint8(), uint16(), uint32(),
uint64(), float32(), float64(), timestamp("ns"), binary(),
large_binary(), fixed_size_binary(32), date32(), date64(),
- decimal(4, 2), dictionary(), struct(x = int32())
+ decimal(4, 2), decimal128(4, 2), dictionary(), struct(x = int32())
Review comment:
I think we can get rid of `decimal(...)` here since it's just a wrapper
for `decimal128()`, right?
##########
File path: r/tests/testthat/test-data-type.R
##########
@@ -395,6 +395,18 @@ test_that("decimal type and validation", {
expect_error(decimal(4, NA), '"scale" must be an integer')
expect_r6_class(decimal(4, 2), "Decimal128Type")
Review comment:
Do we want to also do something like `expect_identical(class(decimal(4,
2)), class(decimal128(4, 2))` that asserts that `decimal()` -> `decimal128()`
##########
File path: r/tests/testthat/test-data-type.R
##########
@@ -395,6 +395,18 @@ test_that("decimal type and validation", {
expect_error(decimal(4, NA), '"scale" must be an integer')
expect_r6_class(decimal(4, 2), "Decimal128Type")
+
+ expect_error(decimal128())
+ expect_error(decimal128("four"), '"precision" must be an integer')
+ expect_error(decimal128(4))
+ expect_error(decimal128(4, "two"), '"scale" must be an integer')
+ expect_error(decimal128(NA, 2), '"precision" must be an integer')
+ expect_error(decimal128(0, 2), "Invalid: Decimal precision out of range: 0")
+ expect_error(decimal128(100, 2), "Invalid: Decimal precision out of range:
100")
+ expect_error(decimal128(4, NA), '"scale" must be an integer')
+
+ expect_r6_class(decimal128(4, 2), "Decimal128Type")
Review comment:
This is super super minor, but maybe we should move this up above the
`expect_errors()`. We aren't strict about this, but _for the most part_ the
pattern we use is test the base cases, test edge cases, test errors (though I'm
sure there are counter examples!)
##########
File path: r/tests/testthat/test-data-type.R
##########
@@ -395,6 +395,18 @@ test_that("decimal type and validation", {
expect_error(decimal(4, NA), '"scale" must be an integer')
expect_r6_class(decimal(4, 2), "Decimal128Type")
+
+ expect_error(decimal128())
Review comment:
Is this an error that the R package raises? If so, should we assert what
it says?
##########
File path: r/tests/testthat/test-data-type.R
##########
@@ -395,6 +395,18 @@ test_that("decimal type and validation", {
expect_error(decimal(4, NA), '"scale" must be an integer')
expect_r6_class(decimal(4, 2), "Decimal128Type")
+
+ expect_error(decimal128())
+ expect_error(decimal128("four"), '"precision" must be an integer')
+ expect_error(decimal128(4))
Review comment:
Is this an error that the R package raises? If so, should we assert what
it says?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscr...@arrow.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
