Cleanly handle HTTP error messages, add stop_on_error and exclude arguments to app$snapshot

[warnes]

I have a troublesome object that changes in unimportant ways from run to run, and don't want to explicitly name all of the other objects that should be included in a snapshot.

This PR provides an 'exclude' argument to app$snapshot that allows specification of object names to be excluded from the snapshot.

It also modifies app$snapshot() to return the processed JSON text, rather than the raw text received from the headless browser.

I have created a 'recorded' test that exercises the new code, and have updated the 'in-depth' vignette.

All of the tests (including the one for new code) succeed, except for one existing 'recorded' test which yields:

── 1. Failure: doc.Rmd (@test-recorded-tests.R#38)  ─────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────
Not all shinytest scripts passed for recorded_tests/rmd-prerendered: mytest

Diff output:
==== mytest ====
diff /tmp/RtmpzMKXCF/working_dir/RtmpYKEARY/shinytest-diff-3aea446621dc/mytest-expected/001.json /tmp/RtmpzMKXCF/working_dir/RtmpYKEARY/shinytest-diff-3aea446621dc/mytest-current/001.json
15,18c15,18
<               "left": 1.456,
<               "right": 5.344,
<               "bottom": -0.0300000000000001,
<               "top": 0.780000000000003
---
>               "left": 1.464,
>               "right": 5.136,
>               "bottom": -0.0340000000000001,
>               "top": 0.884000000000003
diff /tmp/RtmpzMKXCF/working_dir/RtmpYKEARY/shinytest-diff-3aea446621dc/mytest-expected/002.json /tmp/RtmpzMKXCF/working_dir/RtmpYKEARY/shinytest-diff-3aea446621dc/mytest-current/002.json
15,18c15,18
<               "left": 1.564,
<               "right": 5.236,
<               "bottom": -0.0360000000000001,
<               "top": 0.936000000000001
---
>               "left": 1.568,
>               "right": 5.132,
>               "bottom": -0.0440000000000001,
>               "top": 1.144


══ testthat results  ════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════
[ OK: 216 | SKIPPED: 0 | WARNINGS: 0 | FAILED: 1 ]
1. Failure: doc.Rmd (@test-recorded-tests.R#38) 

I suspect this is due to a subtle difference in the configurations of my system and the test system.

[CLAassistant]

All committers have signed the CLA.

[warnes]

sd_snapshot now checks the HTTP code returned from the server. When the new argument stop_on_error is TRUE (the default) the code and error message are extracted from the HTTP response to generate an informative error message. When FALSE, the HTTP response is converted into a proper JSON object and included in the snapshot, and execution continues.

[hadley]

Do you think you could decompose this into two separate PRs? (One handling HTTP failures and the other for excluding values from the snapshot). That would make them easier to review.

[warnes]

Implementing the exclude arguments only involves this code block and adding the omit argument to the sd_snapshot and app$snapshot function definitions and handoff.

[warnes]

Hi Hadley,

I can decompose if you wish, but the changes for handling exclude are so small it's hardly worth it:

1. add the parameter exclude to the snapshot thunk function in app_driver.R
2. add the parameter to the function definition for sd_snapshot
3. add the twelve lines at https://github.com/rstudio/shinytest/pull/324/files#diff-34ed19d4ada8a5949aea349f6013f6b1R64-R75:

if(length(exclude)>0)
  {
    rObj <- jsonlite::fromJSON(txt=content)

    dropItems <- function(l, i) l[! names(l) %in% i]

    rObj$input <- dropItems(rObj$input, exclude)
    rObj$output <- dropItems(rObj$output, exclude)
    rObj$export <- dropItems(rObj$export, exclude)

    content <- jsonlite::toJSON(rObj)
  }

[hadley]

I think it's still useful because that makes the http error handling code easier for me to review (and there's a few style issues that'll need to be resolved along the way).

[hadley]

Actually, there are three places where shinytest currently uses httr::GET() with out checking the returned status, so unless you're keen to do it, I'll take a stab at them either tomorrow or next week.

[warnes]

Right, that's a deliberate choice because I wanted to start simple (and I wasn't sure that shiny allowed you to customise the 404 output in a way that would make sense to test)

The shiny app can return at least 404 and 500, and perhaps other codes, in fact I used PR #348 to track down and fix a 500 error that only occurred when running a shinytest script.

[warnes]

Aside: I think the check failures on Windows are due to pingr::is_up improperly returning TRUE on Windows even if the shiny app isn't listening on the port..