fhdsl / metricminer

R package that digs up data that matters from APIs, making it dashboard-ready
https://hutchdatascience.org/metricminer/
MIT License
2 stars 0 forks source link

Self-sufficient Testing #36

Closed howardbaek closed 9 months ago

howardbaek commented 9 months ago

Purpose/implementation Section

What changes are being implemented in this Pull Request?

After reading the Self-sufficient tests section from the R Packages book, I added withr::local_options() where we grab the Calendly and GitHub tokens from my .Rprofile.

What was your approach?

Used the withr package to make temporary changes in global state. withr::local_options() sets the options and at the end of the test, restores the option to its original value.

Tell potential reviewers what kind of feedback you are soliciting.

I wasn't sure how to address the Google Analytics tests using the same approach since the token for Google Analytics is NOT a string, but a Token2.0 reference class (RC) object. I don't know if/how to store RC objects in my .Rprofile.

howardbaek commented 9 months ago

One way is to run this code chunk in my .Rprofile:

 if (app_name == "google") {
    if (is.null(access_token) || is.null(refresh_token)) {
      stop("For Google auth, need access_token and refresh_token cannot be NULL")
    }
    scopes_list <- unlist(find_scopes(app_name))

    credentials <- list(
      access_token = access_token,
      expires_in = 3599L,
      refresh_token = refresh_token,
      scope = scopes_list,
      token_type = "Bearer"
    )

    token <- httr::oauth2.0_token(
      endpoint = app_set_up(app_name)$endpoint,
      app = app_set_up(app_name)$app,
      cache = cache,
      scope = scopes_list,
      credentials = credentials
    )
  }

But I'm not sure if it is good practice to generate an oauth2.0 token inside .Rprofile, which gets sourced upon startup.

howardbaek commented 9 months ago

I will be reading HTTP testing in R to figure out this problem, which is a rOpenSci project, so perhaps @sckott or @seankross may know?

sckott commented 9 months ago

@howardbaek Is the goal to run tests on CI here on github? Do you want to test the OAuth flow? Or are you not testing the oauth flow, but some other functionality in this pkg?

howardbaek commented 9 months ago

@sckott

Here is the backstory: @cansavvy figured out running testthat tests on GitHub, but I wasn't able to run these tests locally since her tests were using credentials stored on GitHub. While investigating these tests, I found out that the R packages book recommend self-sufficient, self-contained tests.

Eventually, we moved the setup code to tests/testthat/helper.R in commit https://github.com/fhdsl/metricminer/pull/26/commits/99b46e72adfc2683897ef443241a5d12aa6acf1a. Also, in a separate branch corresponding to this PR (howardbaek/local-testing), I included withr::local_options() inside each test_that() code chunk, which grabs the Calendly/GitHub tokens set as env variables inside my .Rprofile and sets them to to withr local options. Then, these withr local options are used by the functions inside the test_that() chunk.

This approach seems to be working for Calendly/GitHub, but not for Google Analytics. Google Analytics credentials is not a string of characters, but a Token2.0 reference class (RC) object, so I didn't know how I could possibly store this object in my an env var...

cansavvy commented 9 months ago

This approach does work for Google but you have to follow a slightly different set up. I've worked on documenting this process in the documentation PR I have open.

cansavvy commented 9 months ago

@howardbaek Is the goal to run tests on CI here on github? Do you want to test the OAuth flow? Or are you not testing the oauth flow, but some other functionality in this pkg?

This set up does test the oauth workflow using secrets. I'm not sure if the way I have it set up is orthodox but it's not dangerous (pretty sure).

howardbaek commented 9 months ago

I tried to address this issue in this commit. Let me know if this makes sense!

sckott commented 9 months ago

to reiterate my points on this topic on our call today

howardbaek commented 9 months ago

After yesterday's meeting, I digged up this testthat vignette. It seems like code in tests/testthat/helper.R is sourced by devtools::load_all() and we put auth_from_secret() inside helper.R. I don't think this is a good idea because auth_from_secret() does the same thing as authorize(), so it makes the authorize() function useless when we are interactively testing our code.

Here is an article that touches on this issue: https://blog.r-hub.io/2020/11/18/testthat-utility-belt/

cansavvy commented 9 months ago

Ah yes we definitely don't want helper.R to be called by devtools::load_all() so we should use something different.

I think the best call based on the collective info here and what we talked about yesterday is to individually set authorizations within tests and use withr so that the options don't stick. I'll push a commit that does what I'm talking about.

cansavvy commented 9 months ago

@howardbaek see if what I've done here is meeting what you are going for. Thanks for bringing up all these excellent points!

howardbaek commented 9 months ago

@cansavvy Thanks for working on this. The changes you made look good to me.