Refactoring

- Move null coalesce to above engine definition - kill_engine to ensure that the nanonext sockets are closed - Move middleware hook to above function definition to avoid scoping issues - Skip script test install for windows due to permission issues
ElianHugh · Jun 17, 2024 · d9bd2cc · d9bd2cc
1 parent 4677320
commit d9bd2cc
Show file tree

Hide file tree

Showing 4 changed files with 51 additions and 33 deletions.
diff --git a/R/engine.R b/R/engine.R
@@ -1,20 +1,27 @@
 new_engine <- function(path, dirs, port, host, ignore) {
+    host <- host %||% "127.0.0.1"
+    ignore <- ignore %||% c("*.sqlite", "*.git*") |>
+        paste0(collapse = "|") |>
+        utils::glob2rx()
+
     list2env(
         list(
             runner = NULL,
             config = list(
-                port = port %||% httpuv::randomPort(),
+                port = port %||% httpuv::randomPort(host = host),
                 path = dirname(path),
                 plumber_path = path,
                 dirs = dirs,
-                host = host %||% "127.0.0.1",
-                ignore = ignore %||% c("*.sqlite", "*.git*") |>
-                    paste0(collapse = "|") |>
-                    utils::glob2rx()
+                host = host,
+                ignore = ignore
             ),
             publisher = nanonext::socket(
                 protocol = "pub",
-                listen = sprintf("ws://%s:%s", host %||% "127.0.0.1", httpuv::randomPort()),
+                listen = sprintf(
+                    "ws://%s:%s",
+                    host,
+                    httpuv::randomPort(host = host)
+                ),
                 autostart = TRUE
             )
         )
@@ -34,10 +41,13 @@ run_engine <- function(engine) {
     cli_welcome()
     buildup_engine(engine)
 
-    current_state <- directory_state(c(
-        engine$config$path,
-        engine$config$dirs
-    ), engine$config$ignore)
+    current_state <- directory_state(
+        c(
+            engine$config$path,
+            engine$config$dirs
+        ),
+        engine$config$ignore
+    )
 
     repeat {
         poll_runner(engine)
@@ -49,6 +59,11 @@ run_engine <- function(engine) {
     }
 }
 
+kill_engine <- function(engine) {
+    nanonext::reap(engine$publisher)
+    kill_runner(engine)
+}
+
 buildup_engine <- function(engine) {
     cli_server_start_progress(engine)
     res <- new_runner(engine)
@@ -64,7 +79,7 @@ buildup_engine <- function(engine) {
 
 teardown_engine <- function(engine) {
     cli_server_stop_progress()
-    resp <- kill_runner(engine)
+    resp <- kill_engine(engine)
     if (isTRUE(resp)) {
         cli::cli_process_done()
     } else {

diff --git a/R/middleware.R b/R/middleware.R
@@ -7,6 +7,7 @@ injection <- function(engine) {
 
 middleware <- function(engine) {
     js <- injection(engine)
+    hook <- postserialise_hotwater(js)
     function(pr) {
         pr |>
             # remove hotwater from the api spec
@@ -20,7 +21,7 @@ middleware <- function(engine) {
                 serializer = plumber::serializer_text(),
                 preempt = "__first__"
             ) |>
-            plumber::pr_hook("postserialize", postserialise_hotwater(js))
+            plumber::pr_hook("postserialize", hook)
     }
 }
 

diff --git a/tests/testthat/test-middleware.R b/tests/testthat/test-middleware.R
@@ -19,23 +19,23 @@ test_that("middleware injection works", {
 test_that("middleware injection works with filters", {
     engine <- new_engine(
         path = system.file("examples", "plumber.R", package = "hotwater"),
-        port = httpuv::randomPort(),
+        port = NULL,
         dirs = NULL,
         host = NULL,
         ignore = NULL
     )
     runner <- callr::r_bg(
-        function(port, middleware) {
-            plumber::pr(system.file("examples", "plumber.R", package = "hotwater")) |>
+        function(config, middleware_filter) {
+            plumber::pr(config$plumber_path) |>
                 plumber::pr_filter("foo", function(req, res) {
                     stop("I break things")
                 }) |>
-                middleware() |>
-                plumber::pr_run(port = port)
+                middleware_filter() |>
+                plumber::pr_run(port = config$port)
         },
         args = list(
-            port = engine$config$port,
-            middleware = middleware(engine)
+            config = engine$config,
+            middleware_filter = middleware(engine)
         )
     )
 
@@ -50,44 +50,45 @@ test_that("middleware injection works with filters", {
         httr2::resp_status()
 
     expect_identical(resp, 200L)
+    kill_engine(engine)
 })
 
 test_that("is_plumber_running works", {
-    dummy_engine <- list(
-        config = list(
-            port = httpuv::randomPort()
-        )
+    engine <- new_engine(
+        path = system.file("examples", "plumber.R", package = "hotwater"),
+        port = NULL,
+        dirs = NULL,
+        host = NULL,
+        ignore = NULL
     )
     router <- callr::r_bg(
-        function(port) {
-            plumber::pr(
-                system.file("examples", "plumber.R", package = "hotwater")
-            ) |>
+        function(config) {
+            plumber::pr(config$plumber_path) |>
                 plumber::pr_get(
                     "/__hotwater__",
                     function() "running",
                     serializer = plumber::serializer_text()
                 ) |>
-                plumber::pr_run(port = port)
+                plumber::pr_run(port = config$port)
         },
         args = list(
-            port = dummy_engine$config$port
+            config = engine$config
         )
     )
     i <- 1L
-    while (i < 20L && !is_plumber_running(dummy_engine)) {
+    while (i < 20L && !is_plumber_running(engine)) {
         i <- i + 1L
         Sys.sleep(0.5)
     }
     router$kill_tree()
-
     expect_lt(i, 20L, label = "loop iterations")
+    kill_engine(engine)
 })
 
 test_that("autoreloader is attached", {
     engine <- new_engine(
         path = system.file("examples", "plumber.R", package = "hotwater"),
-        port = httpuv::randomPort(),
+        port = NULL,
         dirs = NULL,
         host = NULL,
         ignore = NULL
@@ -97,5 +98,5 @@ test_that("autoreloader is attached", {
         httr2::req_perform() |>
         httr2::resp_body_html()
     expect_true(grepl(resp, pattern = "<script>"))
-    kill_runner(engine)
+    kill_engine(engine)
 })
diff --git a/tests/testthat/test-script.R b/tests/testthat/test-script.R
@@ -1,4 +1,5 @@
 test_that("hotwater install/uninstall works", {
+    skip_on_os("windows")
     local({
         hw_install_folder <- withr::local_tempdir("install_path")
         expect_no_error(install_hotwater(hw_install_folder))