better logic? #116

maelle · 2021-03-24T12:50:43Z

@GregSutcliffe

.github/workflows/R-CMD-check.yaml

maelle · 2021-03-24T12:52:58Z

For testing this I deactivated my MEETUPR_PWD, and added a test, using my own token.

maelle · 2021-03-24T12:53:32Z

I only had to run tests to create the fixture so this should fix the flaw you found @GregSutcliffe?

tests/fixtures/get_event_attendees.yml

maelle · 2021-03-24T12:55:08Z

tests/testthat/setup.R

@@ -20,7 +20,9 @@ if (nzchar(Sys.getenv("MEETUPR_PWD"))) {
  meetup_auth(token = temptoken)

 } else {
-  Sys.setenv("MEETUPR_TESTING" = TRUE)
+  if (!identical(Sys.getenv("NOT_CRAN"), "true")) {


on CRAN it's like on CI (apart from the workflow in with-auth.yml) we only want to use the fixtures.

I see where you're going, but the double-negative of !NOT-THING isn't ideal. Can we word it as a positive-case, perhaps set "USE_CRAN: true" in the CI config and then it doesn't have to be explicitly disabled by users?

ouch I think this might be where the problem comes from (although I'd like to keep the idea of this code I took from testthat:::on_cran())

You shouldn't need to worry about NOT_CRAN as it is set by devtools (I just re-read ?testthat::skip_on_cran)

Fair enough. Double-negatives are annoying though, though I doubt testthat would change it given the sheer number of downstream packages that would break ;)

I'm no fan of calling package internals, but would it be better to call

if (testthat:::on_cran) {

To make the double-negative go away and also not duplicate their code into meetupr?

well as it's not exported I don't think we can. 😬

Fair enough. My own package-writing skills are less polished, I wasn't sure if we could access it during tests (obviously it's not relevant to a production install). Duplication it is - although we could copy the on_cran() function to internals.R and then add a link to the source. But I'm nitpicking here, for sure.

maelle · 2021-03-24T13:06:12Z

@GregSutcliffe I'll wait for your feedback and will amend/merge this within the next few days hopefully. Thanks again for identifying the problem!

GregSutcliffe · 2021-03-24T14:46:13Z

Bad news I'm afraid @maelle, I can't get it to work. I added a test to find_groups, and ran it, and it returned 401. Here's my transcript - I think I did it the way you want?

Restarting R session...
* Project '~/git/meetupr' loaded. [renv 0.13.0]
> devtools::load_all()
Loading meetupr

> meetup_auth(token = '~/.local/share/meetupr/meetupr-token.rds')

> Sys.setenv("NOT_CRAN"='true')

> devtools::test()
Loading meetupr
Testing meetupr
✓ |  OK F W S | Context
< snip for brevity >
x |   1 1     | find_groups [0.9 s]
──────────────────────────────
Error (test-find_groups.R:12:3): find_groups() success case 2
Error: Unauthorized (HTTP 401).
Backtrace:
 1. vcr::use_cassette(...) test-find_groups.R:12:2
 3. meetupr::find_groups(text = "Ansible") test-find_groups.R:13:4
< snip for brevity>
[ FAIL 1 | WARN 0 | SKIP 1 | PASS 12 ]

> # confirm token is working
> find_groups('Ansible') %>% nrow()
Auto-refreshing stale OAuth token.
Downloading 324 record(s)...
[1] 400    <- because #113 isn't merged :P

maelle · 2021-03-25T11:41:46Z

Thank you! Trying to find my mistake.

You shouldn't need to run meetup_auth(token = '~/.local/share/meetupr/meetupr-token.rds') before running the tests I think. 🤔 (i.e. if there's no token one would be created) but that's not the problem.

maelle · 2021-03-25T11:44:48Z

my script is that I simply run devtools::test() / the new test script via the button "run tests".

GregSutcliffe · 2021-03-25T11:55:15Z

Hmm. this starts to feel like something is borken in my checkout rather than your code then. Let me copy my token to a clean VM and try again.

maelle · 2021-03-25T11:55:47Z

oh you know it could well be something on my end!! thanks for your patience

GregSutcliffe · 2021-03-25T12:22:36Z

You shouldn't need to run meetup_auth(token = '~/.local/share/meetupr/meetupr-token.rds') before running the tests I think. thinking (i.e. if there's no token one would be created) but that's not the problem.

Ah, thats true, there's a default OAuth provider. I was trying to make sure it was using my token, but I guess that's not needed. One less variable to wory about. Just installing 46746366 devtools dependencies on my VM now :P

maelle · 2021-03-25T12:27:49Z

🤞

maelle · 2021-03-25T12:28:30Z

It think it would use your token? As long as it's in the default location that meetupr finds via rappdirs.

GregSutcliffe · 2021-03-25T12:57:03Z

OK, this is a long wall-o-text, sorry for that - I've documented every step so I can be as clean as possible, in case I missed something. This is on a pre-existing vm which has never used meetupr, and I've created a new user so there's no R cache. The only difference is it's R 3.6.3, but as I'm seeing the same issues as my R 4.0.4 laptop, I hope that's not a problem.

TL'DR - nope, and I think VCR is the issue. Maybe.

root:~# adduser meetuptest
root:~# sudo -u meetuptest -i

# clone code
meetuptest:~$ git clone https://github.com/rladies/meetupr
Cloning into 'meetupr'...

# Patch in the PR
meetuptest:~/meetupr$ curl -L https://github.com/rladies/meetupr/pull/116.patch | git am -
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100   141  100   141    0     0   2168      0 --:--:-- --:--:-- --:--:--  2169
100 18706    0 18706    0     0  92164      0 --:--:-- --:--:-- --:--:-- 92164
Applying: better logic?
Applying: add test
Applying: cran-proofness
Applying: oops

# Install deps and devtools
meetuptest:~$ cd meetupr/
meetuptest:~/meetupr$ R
R version 3.6.3 (2020-02-29) -- "Holding the Windsock"      # This is quite old, a possible problem?

> install.packages('devtools')
Would you like to create a personal library
‘~/R/x86_64-pc-linux-gnu-library/3.6’
to install packages into? (yes/No/cancel) yes               # definitely a clean library, it's been created

> devtools::install_deps()
> devtools::install_dev_deps()

# Check default tests pass
> devtools::test()
Loading meetupr
Testing meetupr
Adding ~/.local/share/meetupr/meetupr-token.rds to .gitignore
httpuv not installed, defaulting to out-of-band authentication
Enter authorization code: /usr/bin/xdg-open: 778: /usr/bin/xdg-open: www-browser: not found

# Seems like it wants to make a token, but we are on a headless VM
# Lets give it a premade token
meetuptest:~/meetupr$ mkdir -p ~/.local/share/meetupr/
meetuptest:~/meetupr$ scp user@host:live-ansible-token ~/.local/share/meetupr/meetupr-token.rds

# Test the token
> devtools::load_all()
> find_groups(text = "R-Ladies", radius = 10)
Auto-refreshing stale OAuth token.
Downloading 1 record(s)...

# Check default tests pass, round 2
> devtools::test()
[ FAIL 0 | WARN 0 | SKIP 1 | PASS 12 ]

# Good, now add the new test
meetuptest:~/meetupr$ vi tests/testthat/test-find_groups.R 

# Add this test to that file, should create a new YAML
test_that("find_groups() success case - new", {
  message('NOT_CRAN is:')
  message(Sys.getenv("NOT_CRAN"))

  vcr::use_cassette("find_groups_new", {
    groups <- find_groups(text = "R-Ladies", radius = 10)
  })

  expect_equal(1, nrow(groups))
})

# Run tests again
> devtools::test()
⠋ |   1       | find_groups
NOT_CRAN is:
true
✖ |   1 1     | find_groups [0.7 s]
──────────────────────────────
Error (test-find_groups.R:14:3): find_groups() success case - new
Error: Unauthorized (HTTP 401).
Backtrace:
 1. vcr::use_cassette(...) test-find_groups.R:14:2
 3. meetupr::find_groups(text = "R-Ladies", radius = 10) test-find_groups.R:15:4
 4. meetupr::.fetch_results(...) /home/meetuptest/meetupr/R/find_groups.R:54:2
 5. ratelimitr:::meetup_call(...) /home/meetuptest/meetupr/R/internals.R:79:2
 8. meetupr:::fun(...)
 9. httr::stop_for_status(req) /home/meetuptest/meetupr/R/internals.R:34:2
──────────────────────────────
[ FAIL 1 | WARN 0 | SKIP 1 | PASS 12 ]

# oh no - is it VCR?
# comment out the VCR lines in the test:
> devtools::test()
Testing meetupr
⠋ |   1       | find_groups
NOT_CRAN is:
true
[ FAIL 0 | WARN 0 | SKIP 1 | PASS 13 ]

Some notes:

I do need my live token, because this is a VM with no display attached, so it cannot open a browser to do the OAuth dance. So I copy my token and test it, as you see. Meetupr find the token in .local without an explicit call to auth, as you said.
The test passes if I comment out the VCR bits - so the issue isn't in meetupr, I don't think. Something in VCR is messing with the authorization process. Below is the YAML it creates, for what it's worth:

http_interactions:
- request:
    method: get
    uri: https://api.meetup.com/find/groups?offset=0&text=R-Ladies&topic_id=&fields=&radius=10
    body:
      encoding: ''
      string: ''
    headers:
      Accept: application/json, text/xml, application/xml, */*
      Authorization: not my bearer token
  response:
    status:
      status_code: 401
      category: Client error
      reason: Unauthorized
      message: 'Client error: (401) Unauthorized'
    headers:
      connection: keep-alive
      content-length: '86'
      content-type: application/json; charset=utf-8
      server: Apache/2.4.39 (Unix) OpenSSL/1.1.1c
      www-authenticate: OAuth realm="https://api.meetup.com/"
      x-meetup-request-id: ab3f29b2-38d4-4dc2-992d-1a255290248c
      content-encoding: gzip
      accept-ranges: bytes
      date: Thu, 25 Mar 2021 12:52:33 GMT
      via: 1.1 varnish
      x-served-by: cache-dca17730-DCA
      x-cache: MISS
      x-cache-hits: '0'
      x-timer: S1616676754.775455,VS0,VE14
      vary: Accept-Encoding,User-Agent,Accept-Language
    body:
      encoding: UTF-8
      file: no
      string: '{"errors":[{"code":"auth_fail","message":"Invalid oauth credentials"}]}'
  recorded_at: 2021-03-25 12:52:33 GMT
  recorded_with: vcr/0.6.0, webmockr/0.8.0

maelle · 2021-03-25T13:16:43Z

Thank you, that's awesome to have all the details.

I have just realized I am using vcr's latest version, I've updated the DESCRIPTION: could you please try again? Sorry.

GregSutcliffe · 2021-03-25T13:27:34Z

Still no good, sorry :(

Again, here's the steps, I think I got it all.

> devtools::install_dev_deps()
4: vcr (0.6.0 -> 4e0c3272b...) [GitHub]
4: webmockr (0.8.0 -> d366de7c0...) [GitHub]
─  building ‘webmockr_0.8.0.tar.gz’
─  building ‘vcr_0.6.5.93.tar.gz’

# Cleanup old YAML or VCR wont record (maybe we should doc this)
meetuptest:~/meetupr$ rm tests/fixtures/find_groups_new.yml

> devtools::test()
Loading meetupr
Testing meetupr
✔ |  OK F W S | Context
⠋ |   1       | find_groups
NOT_CRAN is:
true
✖ |   1 1     | find_groups [0.7 s]                                                                                                                            
────────────────────────────────────
Error (test-find_groups.R:14:3): find_groups() success case - new
Error: Unauthorized (HTTP 401).
Backtrace:
 1. vcr::use_cassette(...) test-find_groups.R:14:2
 3. meetupr::find_groups(text = "R-Ladies", radius = 10) test-find_groups.R:15:4
 4. meetupr::.fetch_results(...) /home/meetuptest/meetupr/R/find_groups.R:54:2
 5. ratelimitr:::meetup_call(...) /home/meetuptest/meetupr/R/internals.R:79:2
 8. meetupr:::fun(...)
 9. httr::stop_for_status(req) /home/meetuptest/meetupr/R/internals.R:34:2
────────────────────────────────────

Very similar YAML produced - string: '{"errors":[{"code":"auth_fail","message":"Invalid oauth credentials"}]}'

It's definitely using the new VCR, I can see the output is way less verbose (all the "HTTP enabled" messages, etc, are gone).

maelle · 2021-03-25T13:33:02Z

and the fixture with the error was deleted before re-running the tests? (just trying to eliminate a possibility)

GregSutcliffe · 2021-03-25T13:41:59Z

Yep, you can see meetuptest:~/meetupr$ rm tests/fixtures/find_groups_new.yml in the output (I know, it's a wall o text). But I think I will add record = 'all' to my test to avoid the issue.

GregSutcliffe · 2021-03-25T14:46:50Z

I can't believe I didn;t think of this earlier, but there's a much cleaner way to show the failure, and be sure it's real, and thats to delete the record for an existing test:

meetuptest:~/meetupr$ R -e 'devtools::test_file("tests/testthat/test-find_groups.R")'
[ FAIL 0 | WARN 0 | SKIP 0 | PASS 1 ]
meetuptest:~/meetupr$ rm tests/fixtures/find_groups.yml 
meetuptest:~/meetupr$ R -e 'devtools::test_file("tests/testthat/test-find_groups.R")'
[ FAIL 1 | WARN 0 | SKIP 0 | PASS 0 ]

I'm going to poke around inside a few functions and see if i can find what's tripping it up. I added print(.state$token) to find_groups, but that didn't help, it just confirms the calls are made with the same token.

maelle · 2021-03-25T15:00:04Z

Thank you! If I can't look again this week, I'll come back to this next week.

GregSutcliffe · 2021-03-25T15:14:40Z

No worries. The only thing I can see so far that is notable is that without the VCR wrapper, the test will print "Auto-refreshing stale OAuth token." when using my token. With VCR enabled, I don't see this. I'm sure that points to something to do with how tokens are refreshed, but I'm struggling to follow it.

maelle · 2021-03-25T15:21:29Z

Aaah that's interesting.

The token should have been refreshed before, in the setup/before it. 🤔

I know you know OAuth well, here's the extent of my own understanding: https://blog.r-hub.io/2021/01/25/oauth-2.0/

GregSutcliffe · 2021-03-25T15:28:24Z

I know you know OAuth well, here's the extent of my own understanding: https://blog.r-hub.io/2021/01/25/oauth-2.0/

Honestly, I don't. I needed to add it here in a hurry last year, so I did, but that's the first and last time I've needed to implement it myself :). That blog looks handy, added to my reading list, thanks!

GregSutcliffe · 2021-03-25T15:55:08Z

OK, so the problem occurs here:

meetupr/R/meetup_auth.R

Line 213 in 2ca1098

httr::config(token = .state$token)

As far as I can tell, VCR is interfering with this - stepping through the code with browser() I can exit this function and refresh the token if I disable VCR, but it bombs here with VCR enabled. However, now we're into a different package, so either they have an issue or I'm passing something weird in.

Still trying to actually see what VCR has changed in the environment, but it's hard to know where to look.

maelle · 2021-03-25T15:58:03Z

So, I feel super bad not looking into this myself now, so don't feel obliged to explore further, but if you're curious, vcr maintainer started writing a vignette about vcr under the hood. ropensci/vcr#233

GregSutcliffe · 2021-03-25T16:42:58Z

Lack of curiosity is not my main problem :)

I'm definitely got the urge to fix it - if neither of us could add a test, I could let it go, but as you can't currently replicate the issue, it's kinda on me to figure out WTH is going on... Hopefully, I can get to the bottom of it.

maelle added 2 commits March 24, 2021 13:36

better logic?

0253994

add test

1f14d4b

maelle commented Mar 24, 2021

View reviewed changes

.github/workflows/R-CMD-check.yaml Outdated Show resolved Hide resolved

cran-proofness

cbcc172

maelle commented Mar 24, 2021

View reviewed changes

tests/fixtures/get_event_attendees.yml Show resolved Hide resolved

maelle commented Mar 24, 2021

View reviewed changes

oops

738f462

oops

e905412

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

better logic? #116

better logic? #116

maelle commented Mar 24, 2021

maelle commented Mar 24, 2021

maelle commented Mar 24, 2021

maelle Mar 24, 2021

GregSutcliffe Mar 24, 2021

maelle Mar 25, 2021

maelle Mar 25, 2021

GregSutcliffe Mar 25, 2021

maelle Mar 25, 2021

GregSutcliffe Mar 25, 2021

maelle commented Mar 24, 2021

GregSutcliffe commented Mar 24, 2021

maelle commented Mar 25, 2021

maelle commented Mar 25, 2021 •

edited

GregSutcliffe commented Mar 25, 2021

maelle commented Mar 25, 2021

GregSutcliffe commented Mar 25, 2021

maelle commented Mar 25, 2021

maelle commented Mar 25, 2021

GregSutcliffe commented Mar 25, 2021

maelle commented Mar 25, 2021

GregSutcliffe commented Mar 25, 2021

maelle commented Mar 25, 2021

GregSutcliffe commented Mar 25, 2021

GregSutcliffe commented Mar 25, 2021

maelle commented Mar 25, 2021

GregSutcliffe commented Mar 25, 2021

maelle commented Mar 25, 2021

GregSutcliffe commented Mar 25, 2021

GregSutcliffe commented Mar 25, 2021 •

edited

maelle commented Mar 25, 2021

GregSutcliffe commented Mar 25, 2021

better logic? #116

Are you sure you want to change the base?

better logic? #116

Conversation

maelle commented Mar 24, 2021

maelle commented Mar 24, 2021

maelle commented Mar 24, 2021

maelle Mar 24, 2021

Choose a reason for hiding this comment

GregSutcliffe Mar 24, 2021

Choose a reason for hiding this comment

maelle Mar 25, 2021

Choose a reason for hiding this comment

maelle Mar 25, 2021

Choose a reason for hiding this comment

GregSutcliffe Mar 25, 2021

Choose a reason for hiding this comment

maelle Mar 25, 2021

Choose a reason for hiding this comment

GregSutcliffe Mar 25, 2021

Choose a reason for hiding this comment

maelle commented Mar 24, 2021

GregSutcliffe commented Mar 24, 2021

maelle commented Mar 25, 2021

maelle commented Mar 25, 2021 • edited

GregSutcliffe commented Mar 25, 2021

maelle commented Mar 25, 2021

GregSutcliffe commented Mar 25, 2021

maelle commented Mar 25, 2021

maelle commented Mar 25, 2021

GregSutcliffe commented Mar 25, 2021

maelle commented Mar 25, 2021

GregSutcliffe commented Mar 25, 2021

maelle commented Mar 25, 2021

GregSutcliffe commented Mar 25, 2021

GregSutcliffe commented Mar 25, 2021

maelle commented Mar 25, 2021

GregSutcliffe commented Mar 25, 2021

maelle commented Mar 25, 2021

GregSutcliffe commented Mar 25, 2021

GregSutcliffe commented Mar 25, 2021 • edited

maelle commented Mar 25, 2021

GregSutcliffe commented Mar 25, 2021

maelle commented Mar 25, 2021 •

edited

GregSutcliffe commented Mar 25, 2021 •

edited