search

rstudio / shinytest

Automated testing for shiny apps
https://rstudio.github.io/shinytest/
Other
225 stars 55 forks source link

Add updatedExpected parameter to testApp. Close #418 #419

Closed bersbersbers closed 2 years ago

bersbersbers commented 2 years ago

I am bit unsure about the best testing strategy. The only test using testApp() that I could find is https://github.com/rstudio/shinytest/blob/master/tests/testthat/test-recorded-tests.R

Should I integrate the updateExpected = TRUE case into that test? Duplicate that test, maybe with just a single expect_recorded_app? Or come up with a failing test first so that the update actually does something?

CLAassistant commented 2 years ago

CLA assistant check
All committers have signed the CLA.

bersbersbers commented 2 years ago

Will the email address I enter in the CLA assistant be made public anywhere?

bersbersbers commented 2 years ago

@schloerke as a recent commiter, can you recommend a reviewer and/or approve workflows, please?

schloerke commented 2 years ago

If we're following the cadence of {testthat}, snapshots can not be auto-accepted. A separate method must be run to perform the action of "snapshot update".

Therefore, I'd like to close this PR to keep the separation of these two actions.

bersbersbers commented 2 years ago

Fine with me. I thought I had a good use case in #418, but if consistency with testthat (which I have very little experience with by itself) is important, so be it.

Do note that the default value would be updateExpected = FALSE, so this would be an option with no impact on standard behavior as far as I can see.