Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adding tests #31

Closed
wants to merge 5 commits into from
Closed

Adding tests #31

wants to merge 5 commits into from

Conversation

andodet
Copy link

@andodet andodet commented Dec 27, 2019

After experimenting with couple different approaches, I believe testing shinyauthr functionalities against the Shiny example app in inst/ would be the most practical way as it allows to test the user-facing interface (leaving room to change the underlying logic, e.g hashing, etc.).

Tests have been structured as:
Login:

  • Login form displays properly (empty user_name and password fields with collapsed sidebar).
  • If a user logs in successfully user_auth is set to TRUE and info gets update with user infos.
  • When a user logs in successfully sidebarCollapsed is set to FALSE and logout button displays.

Logout:

  • Logging out successfully sets user_auth to FALSE and info to NULL
  • Logging out hides logout button and sets sidebarCollapsed to TRUE

Tests run well under a minute (~26secs.), hence it shouldn't be required to skip tests when submitting.

Would be really interesting to understand if I overlooked any better approach to do this.

Help needed

Exporting values is needed in order to inspect them during testing. This is done by adding a call to exportTestValues() in app.R.

  # Export reactive values for testing
  exportTestValues(
    auth_status = credentials()$user_auth,
    auth_info   = credentials()$info,
    user_data    = user_data()  # this throws an error
  )

On the other hand, exporting user_data() reactive data frame breaks the call, trowing the following error (even when in testmode):

Unable to fetch all values from server. Is target app running with options(shiny.testmode=TRUE?)

This is blocking me from testing the right data is being show to user respecting their permission level. Hence tests have been commented out test-shinyauthr.R.

I am probably missing something obvious here but the same call works like a charm on a mock Shiny app.

In order to inspect reactive variables in testing exportTestValues is needed.
Exporting user_data() throws an error, which makes impossible to test if user permission is respected when serving data to user.
Tests are structured according the following logic:
**Login**:
* Login form renders properly.
* Successful login updates user_auth and info.
* Successful login uncollapses sidebar and renders logout button.

**Logout**:
* Succesful logout updates user_auth and info.
* Successful logout collapses sidebar and hides logout button.
@andodet andodet mentioned this pull request Dec 27, 2019
@PaulC91
Copy link
Owner

PaulC91 commented Jan 11, 2020

Thanks so much for this! Sorry I'm super busy at the moment so haven't had time to test yet but will do asap.

Thanks again,
Paul

@andodet
Copy link
Author

andodet commented Jan 11, 2020

Another thing I forgot to mention is that by testing the example Shiny application, devtools::test_coverage() will obviously result in a 0% as we're not testing the core package functions. If it's a deal breaker, other approaches must be explored.

@PaulC91
Copy link
Owner

PaulC91 commented Jul 13, 2021

Hi. Just added some tests in #52 .
Mostly inspired by this PR - thanks for the contribution.
And sorry it took so long!

@PaulC91 PaulC91 closed this Jul 13, 2021
@andodet
Copy link
Author

andodet commented Jul 13, 2021

Hey @PaulC91, no worries - happy it helped in any way. Hope to see the package on CRAN at some point.

@PaulC91
Copy link
Owner

PaulC91 commented Jul 20, 2021

@andodet made it to CRAN! https://cran.r-project.org/package=shinyauthr

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants