chore: use v1 timezones API #315

toph-allen · 2024-10-11T22:58:16Z

Intent

This PR contains two sets of changes, which were interrelated at the start of this PR but are now less so:

Use v1 timezones API when available.
Refactor some server-version-related code.
A different approach to mocking HTTP methods.

Fixes #300

Approach

Time zones

The time zones change is relatively simple.

The server-version-related refactoring is in here because I originally was using the time zones API switch to think about how the logic for choosing a URL would look if it also keyed off the server version. It made the logic much more complicated than was necessary, so I removed that logic from the get_timezones() function, but I have kept some of the version-getting-related refactoring. Another option would be moving it to a different PR.

Server version

The gist of the refactoring is:

Renaming some private functions.
Connect$version is now an active binding on the Connect object, a private property that is NULL by default, and a character vector when fetched. NA indicates that the version is hidden.

This could easily be removed from this PR and tabled or submitted in a separate PR.

Testing

I've been finding it difficult to manage our testing fixtures here — we have 79 files in our fixtures dirs right now, and managing tests when the required context is distributed in that file hierarchy has been a cognitive challenge. For simple unit tests like this, I thought an approach where I stub out the request method in the subclass would provide a few benefits.

Test a variety of different responses from the same endpoint without needing different mock dirs.
Track the exact requests that were made of the object and confirm expected behavior there.

Checklist

Does this change update NEWS.md (referencing the connected issue if necessary)?
Does this change need documentation? Have you run `document()?

- use na instead of null for no value - update function name - add tests for version getting

made possible by caching version on the client object

R/schedule.R

jonkeane · 2024-10-22T22:53:59Z

tests/testthat/test-schedule.R

+MockConnect <- R6Class(
+  "MockConnect",
+  inherit = Connect,
+  public = list(
+    initialize = function(version = NA) {
+      self$server <- "https://connect.example"
+      self$api_key <- "fake"
+      private$.version <- version
+    },
+    request = function(method, url, ..., parser = "parsed") {
+      # Record call
+      self$log_call(paste(method, url))
+
+      # Look for response
+      if (!(url %in% names(self$responses))) {
+        stop("Unexpected URL")
+      }
+      res <- self$responses[[url]]
+
+      if (is.null(parser)) {
+        res
+      } else {
+        self$raise_error(res)
+        httr::content(res, as = parser)
+      }
+    },
+    responses = list(),
+    add_mock_response = function(path, content, status_code = 200L, headers = c("Content-Type" = "application/json; charset=utf-8")) {
+      url <- self$api_url(path)
+      res <- mock_response(url, content, status_code, headers)
+      self$responses[[url]] <- res
+    },
+    call_log = character(),
+    log_call = function(call) {
+      self$call_log <- c(self$call_log, call)
+    }
+  )
+)
+
+mock_response <- function(url, content, status_code, headers = character()) {
+  # Headers in responses are case-insensitive lists.
+  names(headers) <- tolower(names(headers))
+  headers <- as.list(headers)
+  headers <- structure(as.list(headers), class = c("insensitive", class(headers)))
+
+  # Treat content similarly to httr::POST, with a subset of behaviors
+  if (is.character(content) && length(content) == 1) {
+    content <- charToRaw(content)
+  } else if (is.list(content)) {
+    content <- charToRaw(toJSON(content, auto_unbox = TRUE))
+  }
+
+  structure(
+    list(
+      url = url,
+      status_code = status_code,
+      request = structure(list(url = url), class = "request"),
+      headers = headers,
+      content = content
+    ),
+    class = "response"
+  )
+}
+
+mock_response_404 <- function(url) {
+  mock_response(url, content = "404 page not found", status_code = 404L, headers = c("Content-Type" = "text/plain"))
+}
+
+
+mock_response(
+  url = "v1/timezones",
+  body = list(
+    list(timezone = "Africa/Abidjan", offset = "+00:00"),
+    list(timezone = "Africa/Accra", offset = "+00:00")
+  )
+)


Is this something you're having trouble getting httptest to do? This pattern should be possible:

https://github.com/Crunch-io/rcrunch/blob/b709e282172ee6b813b26299c28514ea840c47a6/tests/testthat/test-api.R#L117-L124 and the fixture https://github.com/Crunch-io/rcrunch/blob/b709e282172ee6b813b26299c28514ea840c47a6/tests/testthat/app.crunch.io/404.R#L1-L18 are one example of this.

I know that you mentioned wanting to experiment with possible other test runners / setups for the http testing, but I don't think this is the place we should do that. Let's keep using what we have and defer taking a new testing approach until after we have at least a few PRs on our journey of improving the recipes.

This commit was pushed up in a state I had not intended to receive review. I had a 1-on-1 with @nealrichardson earlier today and asked some questions about the approach to testing that I had taken with this PR, and he gave some thoughts on streamlining it that I implemented after our chat.

I was motivated to explore this approach as I was adding fixtures for the tests yesterday. There are three scenarios I want to test:

Both endpoints respond

The new endpoint returns a 404

The new and old endpoints returns a 404

So far, we have two mock dirs that are used for different versions of Connect. I can use those to mark out the two scenarios, but to test a third scenario requires adding another mock directory, and — I'm loathe to move to a model where we have additional mock dirs for each testing scenario, and managing tests of simple scenarios that are split out across an increasingly complex file tree has been a headache.

I threw this together in a few hours. It seems to work, and I @nealrichardson's suggestions made it much tighter. I also have found the workflow a good fit for me personally, and I think the tests are nicely concise and contained — it feels like a good fit for this kind of narrowly-scoped unit test.

I'm actually really excited about this approach — I feel like I've solved a piece of workflow friction that has been slowing me down.

toph-allen · 2024-10-23T13:57:37Z

R/schedule.R

@@ -337,7 +337,13 @@ schedule_describe <- function(.schedule) {
 #' @family schedule functions
 #' @export
 get_timezones <- function(connect) {
-  raw_tz <- connect$GET(unversioned_url("timezones"))
+  raw_tz <- tryCatch(


@jonkeane One problem using tryCatch is that connectapi emits a message, not just an error, when it hits an HTTP error. This means that even if we catch the error, a message is emitted saying 404 page not found when the incorrect URL is tried. This may have been our original motivation for not parsing the responses in the thumbnail API. I'll add a comment to this thread with a code ref for the error handling.

connectapi/R/connect.R

Lines 79 to 96 in adc2041

raise_error = function(res) {

if (httr::http_error(res)) {

err <- sprintf(

"%s request failed with %s",

res$request$url,

httr::http_status(res)$message

)

tryCatch(

{

message(capture.output(str(httr::content(res))))

},

error = function(e) {

message(e)

}

)

stop(err)

}

},

I'll go ahead and create an issue to track this so we don't need to treat it as blocking this work.

[EDIT] #317

@toph-allen and I discussed this over Zoom. Since it isn't directly relevant to this feature, we're going to create a separate issue to capture the oddities.

Generally speaking, a console write should always be accomplished through a logging interface. That way the user has some control over the messages (e.g.., disabling them). And even then, a logging interface is usually overkill. In my opinion, this type of message should just be part of the raised error. Then the program can handle the error as needed.

tdstein · 2024-10-23T15:48:36Z

tests/testthat/test-schedule.R

+  client$mock_response(
+    path = "v1/timezones",
+    content = "404 page not found",
+    status_code = 404L,
+    headers = c("Content-Type" = "text/plain; charset=utf-8")
+  )


I personally like this abstraction quite a bit. It removes a lot of the boilerplate and clarifies what's being augmented for the purpose of the test.

Also, This looks fairly similar to the responses package we are using in Python.

tdstein

Implementation looks good to me. Have we had a chance to test this against live instances of the relevant Connect versions?

toph-allen · 2024-10-23T15:58:29Z

I've tested it against Dogfood. But -- what I also am reminded is that I should try to add an integration test for this.

toph-allen · 2024-10-23T19:36:13Z

The get_timezones() function failed in the integration tests. The test was previously commented out, and I tried a different approach, but I suspect there might be something about GitHub Actions runners which make it fail.

I manually built Connect 2024.08.0 locally and tested against that to manually verify against relevant versions.

tests/testthat/test-schedule.R

tests/testthat/setup.R

tests/integrated/test-schedule.R

Co-authored-by: Neal Richardson <[email protected]>

toph-allen added 3 commits October 11, 2024 18:54

first pass at new time zone URL

8ad8eb9

server version updates

823862e

- use na instead of null for no value - update function name - add tests for version getting

refactor error_if_less_than to use version number

7762c7a

made possible by caching version on the client object

jonkeane reviewed Oct 21, 2024

View reviewed changes

R/schedule.R Outdated Show resolved Hide resolved

toph-allen added 4 commits October 21, 2024 19:24

add tests for timezones using subclasses to mock

eed331f

simplify function, update tests

6918af8

formatting changes

18e2c8e

refactor tests

d31c496

jonkeane reviewed Oct 22, 2024

View reviewed changes

toph-allen added 2 commits October 22, 2024 19:22

clean up

3fb399f

documentation

5ffd793

toph-allen requested review from nealrichardson and jonkeane October 22, 2024 23:35

news

adc2041

toph-allen force-pushed the toph/timezones-v1 branch from a7f7b1d to adc2041 Compare October 22, 2024 23:45

toph-allen marked this pull request as ready for review October 23, 2024 05:49

cleanup

1fb56f4

toph-allen commented Oct 23, 2024

View reviewed changes

tdstein reviewed Oct 23, 2024

View reviewed changes

tdstein approved these changes Oct 23, 2024

View reviewed changes

toph-allen added 2 commits October 23, 2024 14:57

integration test

4e7bb6d

disable integration test

de718e7

nealrichardson reviewed Oct 23, 2024

View reviewed changes

tests/testthat/test-schedule.R Outdated Show resolved Hide resolved

tests/testthat/setup.R Outdated Show resolved Hide resolved

tests/integrated/test-schedule.R Outdated Show resolved Hide resolved

toph-allen and others added 2 commits October 23, 2024 17:01

Update tests/testthat/test-schedule.R

b2488ed

Co-authored-by: Neal Richardson <[email protected]>

in-line mocks now fully specify route with verb

480fb0f

toph-allen requested a review from nealrichardson October 23, 2024 21:10

toph-allen merged commit cf2b801 into main Oct 23, 2024
19 checks passed

toph-allen deleted the toph/timezones-v1 branch October 23, 2024 22:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: use v1 timezones API #315

chore: use v1 timezones API #315

toph-allen commented Oct 11, 2024 •

edited

Loading

jonkeane Oct 22, 2024

toph-allen Oct 22, 2024

toph-allen Oct 23, 2024

toph-allen Oct 23, 2024

toph-allen Oct 23, 2024 •

edited

Loading

tdstein Oct 23, 2024

tdstein Oct 23, 2024

tdstein left a comment •

edited

Loading

toph-allen commented Oct 23, 2024

toph-allen commented Oct 23, 2024

	raise_error = function(res) {
	if (httr::http_error(res)) {
	err <- sprintf(
	"%s request failed with %s",
	res$request$url,
	httr::http_status(res)$message
	)
	tryCatch(
	{
	message(capture.output(str(httr::content(res))))
	},
	error = function(e) {
	message(e)
	}
	)
	stop(err)
	}
	},

chore: use v1 timezones API #315

chore: use v1 timezones API #315

Conversation

toph-allen commented Oct 11, 2024 • edited Loading

Intent

Approach

Time zones

Server version

Testing

Checklist

jonkeane Oct 22, 2024

Choose a reason for hiding this comment

toph-allen Oct 22, 2024

Choose a reason for hiding this comment

toph-allen Oct 23, 2024

Choose a reason for hiding this comment

toph-allen Oct 23, 2024

Choose a reason for hiding this comment

toph-allen Oct 23, 2024 • edited Loading

Choose a reason for hiding this comment

tdstein Oct 23, 2024

Choose a reason for hiding this comment

tdstein Oct 23, 2024

Choose a reason for hiding this comment

tdstein left a comment • edited Loading

Choose a reason for hiding this comment

toph-allen commented Oct 23, 2024

toph-allen commented Oct 23, 2024

toph-allen commented Oct 11, 2024 •

edited

Loading

toph-allen Oct 23, 2024 •

edited

Loading

tdstein left a comment •

edited

Loading