CKAN test server settings for testthat suite

[florianm]

Including branch 11-resource-update, this branch tries to set CKAN test server settings (url, key, dataset id, resource id) and (not quite succeeds to) use them in tests, currently in test-resource_update and test-ds_dataset_create. I'd be grateful for feedback on how to make testthat pull those settings from ckanr_options or options. Or should we pollute Sys.env with these settings?

This PR is for discussion only, as it contains branch 11-resource-update! This PR addresses #30

[wush978]

Sorry that I am late.

I do not understand why we need two systems to control the behavior of ckanr. We should make the testing environment as close as the real environment, right?

[sckott]

But isn't the server @florianm set up a CKAN server like any other?

[florianm]

@wush978 @sckott not just like any other server - the one and only CKAN providing user-friendly geo-referencing.

Jokes aside, my reasoning for using separate test server settings, and also using my own CKAN instance was convenience to develop and test against a latest source install (v2.4a) without writing to a production CKAN.

[sckott]

@wush978 the majority of tests are still against either http://data.techno-science.ca or http://demo.ckan.org with just some against @florianm server

That okay? Good reason to change what we test against?

[wush978]

Oh, I want to argue that why we should use set_ckan_url and set_api_key for switching the environment to test server. IMO, use a different API to set the url and api for testing environment is redundant and introduce more complexity of maintenance. For developing, I usually use a R session for developing, and I think it is a recommended way because the working space is separated from production working space.

Or maybe what we need is a handle of connection or an object of configuration.

[sckott]

@wush978 What do you mean by

using an R session for developing

And I don't know what these mean

Or maybe what we need is a handle of connection or an object of configuration

All commits/PR's/new branches are tested against Travis-CI, so we need remote CKAN servers to test against. When I run tests locally, they are run just like they are on Travis, against remote servers. I'm not sure how your desired setup differs.

[wush978]

@sckott , thanks for your reply. My previous posts are not clear.

I want to discuss the need of set_test_env and related functions because recently they introduce some problems.

In #31 (comment), I am wondering the difference of environment set by set_test_env and set by functions like set_ckan_url. They must be different or we have no reason to introduce set_test_env.

And @florianm mentioned the convenience of testing, so I want to argue it in #31 (comment).

[florianm]

In my view, set_test_env() providing all test settings for testing purposes separates the developer/tester audience from end users using set_ckanr_url() and set_api_key(). Also, two extra parameters (an existing dataset and resource ID) are provided centrally.
So I wrote set_test_env() to set the four required parameters for testing in a single function.

I agree with @sckott on having separate test and production settings, but I like @wush978 's idea of aligning setting/getting them. If I understood both of you correctly, I can satisfy all of our requirements by replacing my set_test_env() with a new function setup_ckanr():

#' Configure default CKAN settings
#' @param url A CKAN URL (optional), default: "http://data.techno-science.ca"
#' @param key A CKAN API key (optional)
#' @param test_url (character) A valid CKAN URL for testing purposes
#' @param test_key (character) A valid CKAN API key privileged to create datasets at URL
#' @param test_did (character) A valid CKAN dataset ID, existing at url
#' @param test_rid (character) A valid CKAN resource ID, attached to did
setup_ckanr <- function(
url="http://data.techno-science.ca/", #"my production CKAN URL", 
key=NULL, #"my production CKAN API key",
test_url=NULL, #"my test CKAN URL",
test_key=NULL, #"my test CKAN API key",
test_did=NULL, #"my test CKAN existing package ID",
test_rid=NULL #"my test CKAN existing resource ID"){
  Sys.setenv(CKANR_DEFAULT_URL = url)
  Sys.setenv(CKANR_DEFAULT_KEY = key)
  Sys.setenv(CKANR_TEST_URL = test_url)
  Sys.setenv(CKANR_TEST_KEY = test_key)
  Sys.setenv(CKANR_TEST_DID = test_did)
  Sys.setenv(CKANR_TEST_RID = test_rid)
}

Re API key: I'd like to use Sys.setenv(CKANR_DEFAULT_KEY) to align environment variable names, and to avoid colliding with the odd user setting their own CKAN API key system-wide (you never know) and adjust api_key() to set the headers from Sys.getenv(CKANR_DEFAULT_KEY, "").

Usage:

# Unprivileged read-only user could run either of:
setup_ckan("http://data-demo.dpaw.wa.gov.au/")
setup_ckan(url="http://data-demo.dpaw.wa.gov.au/")
set_ckanr_url("http://data-demo.dpaw.wa.gov.au/")

# Privileged CKAN editor/admin user can run either of:
setup_ckan("http://data-demo.dpaw.wa.gov.au/", "some-CKAN-API-key")
setup_ckan(url="http://data-demo.dpaw.wa.gov.au/", key="some-CKAN-API-key")
set_ckanr_url("http://data-demo.dpaw.wa.gov.au/"); set_api_key("some-CKAN-api-key")

# ckanR developer/tester can run either of:
setup_ckan("http://data-demo.dpaw.wa.gov.au/", "some-CKAN-API-key", 
"http://test-ckan.dpaw.wa.gov.au/","test-ckan-API-key",
"test-ckan-dataset-id","test-ckan-resource-id")
setup_ckan(url="http://data-demo.dpaw.wa.gov.au/", key="some-CKAN-API-key", 
test_url="http://test-ckan.dpaw.wa.gov.au/",test_key="test-ckan-API-key",
test_did="test-ckan-dataset-id",test_rid="test-ckan-resource-id")

set_ckanr_url("http://data-demo.dpaw.wa.gov.au/"); set_api_key("some-CKAN-api-key")
set_test_env(url="http://test-ckan.dpaw.wa.gov.au/", key="test-ckan-API-key",
did="test-ckan-dataset-id", rid="test-ckan-resource-id")

# this would work, but have no effect:
setup_ckan()
# setup_ckan should really message the user about what has (and hasn't) been changed.

In all cases, setup_ckan() is the shortest way.

If that's a good idea, the implications would be:

• write tests for setup_ckan
• remove set_test_env() and replace references in docstrings with setup_ckan()
• what should be done with set_ckanr_url(), and set_api_key()?

Also I just noticed:

• the docstring of ckanr_options calls the test URL url,
• the "options" part of ckanr_options stems from our option-setting past while Sys.env is used now.
• would "ckanr_settings" be a better name for ckanr_options?

@wush978 thanks for the inspiration! Does my suggestion for setup_ckanr look cleaner than set_test_env()?
@sckott let me know whether you'd like this comment sent as PR.

[florianm]

got excited, made #37

[wush978]

Dear @sckott and @florianm ,

I think some helper functions for convenience are great, but they should stand on setup functions for normal users such as set_ckanr_url. For example:

setup_cache <- new.env()
setup_ckanr <- function(url, key, rid) {
  # export original setting
  setup_cache$url <- get_ckanr_url()
  ...
  # set the environment in NORMAL way
  set_ckanr_url(url)
}

restore_ckanr <- function() {
  set_ckanr_url(setup_cache$url)
}

IMO, there are two reasons to use normal setup function such as set_ckanr_url inside the helper functions:

1. We might avoid some problems with api key because the testing api key is set as usual api key.
2. This approach is closer to real environment because the normal user should not use set_test_env to set the url and api key. Once if some functions only fail when the default argument is used (maybe due to a typo of the environment variable name), the test scripts based on set_test_env might not be able to detect it.

Well, I just suggest this because you have written some scripts already. But if we could not resolve some issues related to two setup systems such as api key, maybe we could switch back to this approach.

[sckott]

I guess the question is: Do users of this package need access to setting their own URL/key/did/etc. for testing? If not, then maybe we only give users ability to set

• default URL
• api key

and that's it.

So then we could use an internal function that's not exported to modify any settings for our unit tests - I don't think users need to change testing settings, since if test work against CKAN server X they should work for CKAN server Y, right (assuming they're running the same version of CKAN)?

thoughts @florianm @wush978

[florianm]

@sckott IMHO testers should be able to change their test settings.
But that's only my personal spin on things, based on my own requirements - I'm happy to go with whatever makes most sense to you all.

My thoughts / arguments pro #37 (which I only wrote to clarify my own thoughts and provide a tangible subject for discussion) :

• Transparency - makes testing easier, more transparent, lowers entry barrier for users turning contributors (incl good testing), treats users as
• Simplicity - I believe Streamlined setting environment variables into setup_ckanr, superseding set_test_env #37 is as parsimonious as it gets, even for just setting the CKAN default URL (plusminus default API key) - even simpler than set_ckanr_url() and set_api_key().
• Test Robustness - taking my CKAN instance out of the critical path by allowing other settings. Test credentials should not be hard-coded as I cannot guarantee that my server will be online and on that URL forever.

My spin on ckanr's user base:
I'd like to treat users certainly as customisers (using ckanr's functions in a bigger context or their own functions or applications) / potential developers (if they find something of general interest) / possible testers (if testing is easy enough). I'm a user turned contributor myself and found it tricky to test properly at the beginning. I believe getting tests to work is now easier than before.

If we were to go down the lines of #37 I'd suggest we're only a short paragraph in the vignette (with a complete contributing and testing workflow) away from making testing for newcomers to R package development like me easy and fun.

[wush978]

@sckott I agree with you.

@florianm We could fulfill your idea and keep things simple at the same time. What you need is some functions for switching url, api key and other configurations. Having such kind of functions is good, but these functions should call set_ckanr_url and set_api_key internally to decrease the complexity of the maintenance.

IMO, the requirement of key parameter for ckan_POST ( #14 ) is a counter example of having two configuration system ( set_ckanr_url and Sys.getenv(CKANR_TEST_URL) ).

[florianm]

@wush978 I don't fully understand why we need to detour through
set_ckanr_url and set_API_key when setup_ckanr can do the same operations,
plus set the four additional required test settings in the same way.

Assuming for argument's sake we'd keep setup_ckanr, we could and should
remove the other two setters so maintenance happens in one place.

That leaves us with one simple setter for all settings and individual
getters for each setting plus the print.ckanr_settings for a complete dump.
That's as granular as I needed it to be - does that work for you as well?

Edit: added to #37, see my comments there! Look especially at examples and tests to judge whether settings are handled simpler and more consistent. @wush978 thanks for focusing my thinking and helping to bring consistency into my contributions!

On 21/05/2015 8:12 pm, "Wush Wu" notifications@github.com wrote:

@sckott https://github.com/sckott I agree with you.

@florianm https://github.com/florianm We could fulfill your idea and
keep things simple at the same time. What you need is some functions for
switching url, api key and other configurations. Having such kind of
functions is good, but these functions should call set_ckanr_url and
set_api_key internally to decrease the complexity of the maintenance.

IMO, the requirement of key parameter for ckan_POST ( #14
#14 ) is a counter example of
having two configuration system ( set_ckanr_url and
Sys.getenv(CKANR_TEST_URL) ).

—
Reply to this email directly or view it on GitHub
#31 (comment).