`expect_running` either misleading or incorrectly passing

[jamieRowen]

For reproduceable example:

golem::create_golem("test")
golem::use_recommended_tests()

Changed nothing but the app_server source,

app_server <- function( input, output, session ) {
  # Your application server logic 
  stop("")
}

The recommended test of expect_running passes in these conditions. Now this is either expected behaviour but misleading in the sense that the background R process may well be running but the shiny app itself is definitely not, or this is no the expected behaviour of the test.

My reprex package attached in zip
golem_expect_running.zip

[ColinFay]

Mmh, yeah I think there is a confusion in what this test considers as a running app.

Technically speaking, even with stop(""), the process launching the application is in a running state.

If you do Rscript -e "golem::run_dev()" inside your project, you'll see that the process doesn't end, and the app is still running (this stop("") here is obviously a mistake, but there are many server errors that are context dependent).

Let's say on the other hand that you have a coding mistake inside R/plop.R, which contains :

x <- function(){

Then Rscript -e "golem::run_dev()" will fail:

Rscript -e "golem::run_dev()"
ℹ Loading golemdead
Error in parse(text = lines, n = -1, srcfile = srcfile) : 
  /Users/colin/Downloads/golemdead/R/plop.R:2:0: unexpected end of input
1: x <- function(){
   ^
── Error documenting your package ─────────────────────────────────────────────────────────────────────────────
Error in run_app() : could not find function "run_app"
Calls: <Anonymous> -> eval -> eval
Execution halted

So, a running app can throw errors from the server-side, what expect_running() does is checking that R can launch the app: you can encounter a bunch of cases where your app throws an error (and becomes grey), then if you refresh your browser, a fresh state is shown : in that case, the app is still a running app.

Do you think we need to rename this function?
There is probably a clarification to be made inside the documentation.

[jamieRowen]

I think a rename is probably sensible, I came across this problem as I wanted a nice way to detect whether an app has crashed during a test when you change an input for example and realised that is_alive() wasn't doing what I expected.

the example you give above is something I would normally pick up with a lintr

I think if it were completely clear that expect_running or whatever it's new name is doesn't check that the app itself is running then that would help.

As an aside, I don't suppose you have a good solution for checking whether an app itself is actually still running in the sense of not errored and crashed the process, it's turning out to be more difficult to accurately detect than I anticipated.

[ColinFay]

Back to this, I feel like this should be renamed "expect_launchable", instead of "expect_running".