paleolimbot commented on code in PR #13542: URL: https://github.com/apache/arrow/pull/13542#discussion_r926753854
########## r/tests/testthat/helper-filesystems.R: ########## @@ -0,0 +1,188 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. + +#' Run standard suite of integration tests for a filesystem +#' +#' @param name Name of filesystem to be printed in test name +#' @param fs A `FileSystem` instance to test with +#' @param path_formatter A function that takes a sequence of path segments and +#' returns a absolute path. +#' @param uri_formatter A function that takes a sequence of path segments and +#' returns a URI containing the filesystem scheme (e.g. 's3://', 'gs://'), the +#' absolute path, and any necessary connection options as URL query parameters. +test_filesystem <- function(name, fs, path_formatter, uri_formatter) { + # NOTE: it's important that we label these tests with name of filesystem so + # that we can differentiate the different calls to these test in the output. + test_that(sprintf("read/write Feather on %s using URIs", name), { + write_feather(example_data, uri_formatter("test.feather")) + expect_identical(read_feather(uri_formatter("test.feather")), example_data) + }) + + test_that(sprintf("read/write Feather on %s using Filesystem", name), { + write_feather(example_data, fs$path(path_formatter("test2.feather"))) + expect_identical( + read_feather(fs$path(path_formatter("test2.feather"))), + example_data + ) + }) + + library(dplyr) Review Comment: I get why you do this, but it's very weird to `library()` anything within a function because it modifies the global state. I would just check that the namespace has already been attached (`if (!("package:dplyr" %in% search()) abort("library(dplyr) required for test_filesystem()")` should do it). ########## r/tests/testthat/test-gcs.R: ########## @@ -58,3 +58,53 @@ test_that("GcsFileSystem$create() input validation", { 'Invalid options for GcsFileSystem: "role_arn"' ) }) + +if (system('python -c "import testbench"') == 0) { + testbench_port <- Sys.getenv("TESTBENCH_PORT", "9001") + + pid_minio <- sys::exec_background("python", c("-m", "testbench", "--port", testbench_port), + std_out = FALSE, + std_err = FALSE # TODO: is there a good place to send output? + ) + withr::defer(tools::pskill(pid_minio)) Review Comment: Do we use `withr::defer()` anywhere else? We tend to use `on.exit(..., add = TRUE)` elsewhere (if we really need `withr::defer()` then go for it, I just don't know what it does). ########## r/tests/testthat/test-gcs.R: ########## @@ -58,3 +58,53 @@ test_that("GcsFileSystem$create() input validation", { 'Invalid options for GcsFileSystem: "role_arn"' ) }) + +if (system('python -c "import testbench"') == 0) { Review Comment: Can you do ``` skip_on_cran() skip_if_not(system('python -c "import testbench"') == 0) # no if/else needed ``` (A bit more idiomatic an will give you a decent skip message) -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
