check return type of pbGetDevices #36

Merged
merged 3 commits into from Jan 30, 2017

Projects

None yet

3 participants

@restonslacker
Contributor
restonslacker commented Jan 30, 2017 edited

cleaned up a couple of small formatting issues and addressed #35


  • check return code from pbGetDevices()
  • renamed .check_return_code() and .create_push() to .checkReturnCode() and .createPush() to match package style
  • moved .checkReturnCode() to init.R
  • pbSetup() returns an error if there are no devices associated with the provided key (usually this will be caused by a bad key)
  • fixed typo in pbSetup() documentation
> pbSetup("sadf", conffile = "test.json")
Error in pbSetup("sadf", conffile = "test.json") : 
  no devices found for sadf
In addition: Warning message:
401: Unauthorized - No valid access token provided. 
restonslacker added some commits Jan 20, 2017
@restonslacker restonslacker Added new setup function. Other documentation changes are due to newe…
…r version of roxygen2.
d4ff087
@restonslacker restonslacker Merge branch 'master' into feature/setup
# Conflicts:
#	DESCRIPTION
#	R/setup.R
2c86db8
@restonslacker restonslacker ** check return code from pbGetDevices()
** renamed .check_return_code() and .create_push() to .checkReturnCode() and .createPush() to match package style
** moved .checkReturnCode() to init.R
** pbSetup() returns an error if there are no devices associated with the provided key (usually this will be caused by a bad key)
** fixed typo in pbSetup() documentation
fcb91d0
@codecov-io

Codecov Report

❗️ No coverage uploaded for pull request base (master@36e0cc7).


Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 36e0cc7...fcb91d0. Read the comment docs.

@eddelbuettel
Owner

Thanks -- will review. Appreciate the snake_case to camelCase switch :)

@@ -47,6 +47,9 @@ pbSetup <- function(key, conffile) {
pdgd <- pbGetDevices(key)
+ if(!length(pdgd$devices)){
@eddelbuettel
eddelbuettel Jan 30, 2017 Owner

Given that length() returns a number I usually test for zero. But not a biggie.

@eddelbuettel
Owner

I'll merge and do some light editing later.

@eddelbuettel eddelbuettel merged commit cadc260 into eddelbuettel:master Jan 30, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@restonslacker
Contributor

Great! Really looking forward to 0.3.0. Let me know if I can do anything else to help.

@eddelbuettel
Owner

Yep. Just pushed 0.2.0.2. Will run one or two more checks with r-devel but is probably ready.

$ Rscript -e 'RPushbullet::pbPost(type=praise::praise())'
@eddelbuettel
Owner

Anything left to do? Or good to ship as is?

@restonslacker
Contributor

Update the version number in the DESCRIPTION file. ;)

I think we're good.

@eddelbuettel
Owner

:)

Had one pending ChangeLog entry too. Will run one more set of tests, and then ship tomorrow morning,

@eddelbuettel
Owner

Ugggh. Ran tests "one more time" and of course something bites. Taking a look. If you're around, maybe poke as well...

@restonslacker
Contributor
@eddelbuettel
Owner

I give up. It is somewhere in the new curl handling code. Some setup code fails.

In the simpleTest.R, the first three pass but as soon as it branches into if (hasDevices) on line 79, the following one fails. Executing by hand shows it more clearly. It looks like the default device is not picked right, and it likely one again list vs vectors from the jsonlite switch.

@eddelbuettel
Owner

Example:

R> str(fromJSON(pbPost(type="file", url=file, recipients = 1)[[1]]))
List of 2
 $ error     :List of 5
  ..$ code   : chr "invalid_param"
  ..$ type   : chr "invalid_request"
  ..$ message: chr "The param 'device_iden' has an invalid value."
  ..$ param  : chr "device_iden"
  ..$ cat    : chr ">:3"
 $ error_code: chr "invalid_param"
Warning message:
400: Bad Request - Usually this results from missing a required parameter. 
R> 
@eddelbuettel
Owner

I just pushed some smaller pending cleanups. It would be nice if you could sync with master and branch from current head. We were a little all over the places as there was so much activity.

To see, use eg these aliases in ~/.gitconfig and run git ls

[push]
#       default = simple
# cf http://stackoverflow.com/a/948397/143305
        default = current
[color]
        ui = true
[alias]
        st = status
        ci = commit
        co = checkout
        ls = log --color --graph --pretty=format:'%Cred%h%Creset -%C(yellow)%d%Creset %s %Cgreen(%cr) %C(bold blue)<%an>%Creset' --abbrev-commit
        ll = log --pretty=format:\"%C(yellow)%h%Cred%d %Creset%s%Cblue [%cn]\" --decorate --numstat
        hist = log --graph --decorate --pretty=oneline --abbrev-commit
        pu = pull --all --prune
edd@max:~/git/rpushbullet(master)$ 
@restonslacker
Contributor

Sorry about the git mess. That was me being thick when I merged your master branch into my old feature/setup which was originally cut for #27. i should have just worked from a new branch or your feature/add_setup branch.

WRT the tests, you were right, toJSON() was writing vectors of length 1 as lists so instead of {"key":"...my key..."} it was writing {"key":["...my key..."]}. I set toJSON(reslist, auto_unbox=TRUE) and the tests pass for me.

@eddelbuettel
Owner

Re git: Yeah. I always use fresh branches of master, in a classic 'shortlived branches of trunk' model. It find that easier to follow in the long run, It gets hairy quickly. It had happened here before.

Re toJSON. What a pain. Then either pbSetup needs to be fixed (and now you see why I was hesitating to go there....) and/or the pbPush code needs to be using more accessor helper functions that work with two different setup files!

@eddelbuettel
Owner

Re git: Much better now with #37. One issue (still seen when to do git ls with eg the alias I sent you) is that your git comment is not following convention. It is pretty much standard to have one short line (I think it may be 50 char, my main git client [magit for emacs] warns with different color if it is longer) followed by one empty line and then possible essays as you do. GitHub will render that better too.

@eddelbuettel
Owner

With that being said, can you look into why the tests are failing? With my resource file, I do test 1 to 3 and some later ones but the errors I quoted above on 4 and 5 (and possibly more later).

@restonslacker
Contributor

Thanks for the info. All my training is in operations research so I'm not a super strong developer, but I try to conform to best practices when I know about them.

@eddelbuettel
Owner

So. In eg RStudio or ESS or whatever you use, execute simpleTests.R line by line. When I then "jump" over the whole if block for lines 79 to 99 I get

Warning messages:
1: 400: Bad Request - Usually this results from missing a required parameter. 
2: 400: Bad Request - Usually this results from missing a required parameter. 
3: 400: Bad Request - Usually this results from missing a required parameter. 
4: 400: Bad Request - Usually this results from missing a required parameter. 
R> 

I shouldn't. I have several devices (phone, tablet, chrome).

@eddelbuettel
Owner

Just running lines 82 and 83 gets me

R> file <- testfile(count(TRUE))
R> str(fromJSON(pbPost(type="file", url=file, recipients = 1)[[1]]))
List of 2
 $ error     :List of 5
  ..$ code   : chr "invalid_param"
  ..$ type   : chr "invalid_request"
  ..$ message: chr "The param 'device_iden' has an invalid value."
  ..$ param  : chr "device_iden"
  ..$ cat    : chr ">:3"
 $ error_code: chr "invalid_param"
Warning message:
400: Bad Request - Usually this results from missing a required parameter. 
@eddelbuettel
Owner

Oh kill me now. If I set a default device (as your pbSetup() then even 1 fails for me now

R> res <- fromJSON(pbPost("note", count(title),
+                            "We think this should work.\nWe really do.")[[1]])
 Show Traceback
 
 Rerun with Debug
 Error in if (d == 0) tgt <- list() else tgt <- list(device_iden = devices[[d]]) : 
  missing value where TRUE/FALSE needed 

What a clusterfuck.

@restonslacker
Contributor

Ok. I'm just not sure what's happening because so far it works for me. Below is the complete log where I use my normal ~/.rpushbullet.json file and then replace it with one created by pbSetup(). Both seem to work for me (just running lines 82 & 83 at the moment).

> file <- testfile(count(TRUE))
> str(fromJSON(pbPost(type="file", url=file, recipients = 1)[[1]]))
List of 18
 $ active                   : logi TRUE
 $ iden                     : chr "ujCIRYHI"
 $ created                  : num 1.49e+09
 $ modified                 : num 1.49e+09
 $ type                     : chr "file"
 $ dismissed                : logi FALSE
 $ direction                : chr "self"
 $ sender_iden              : chr "ujR6G"
 $ sender_email             : chr "wenchel@gmail.com"
 $ sender_email_normalized  : chr "wenchel@gmail.com"
 $ sender_name              : chr "Seth Wenchel"
 $ receiver_iden            : chr "ujCIR6G"
 $ receiver_email           : chr "wenchel@gmail.com"
 $ receiver_email_normalized: chr "wenchel@gmail.com"
 $ target_device_iden       : chr "ujEFi0"
 $ file_name                : chr "test_file_for_post_12.txt"
 $ file_type                : chr "text/plain; charset=utf-8"
 $ file_url                 : chr "https://dl2.pushbulletusercontent.com/mqTkcwuazxm/C:%5CUsers%5Cssw26%5CAppData%5CLocal%5CTemp%5CRtmpWqvRMM"| __truncated__
> 
> 
> file.rename("~/.rpushbullet.json", "~/.rpushbullet.json_orig")
[1] TRUE
> pbSetup(apikey = RPushbullet:::.getKey(), conffile = "~/.rpushbullet.json")
[1] "1. seth's iPad"
[1] "2. Da's comm thingy"
[1] "3. Firefox"
Select a default device (0 for none): 3
> 
> search()
 [1] ".GlobalEnv"          "package:jsonlite"    "package:RPushbullet" "tools:rstudio"       "package:stats"      
 [6] "package:graphics"    "package:grDevices"   "package:utils"       "package:datasets"    "package:methods"    
[11] "Autoloads"           "package:base"       
> detach(3, unload=TRUE)
> search()
 [1] ".GlobalEnv"        "package:jsonlite"  "tools:rstudio"     "package:stats"     "package:graphics"  "package:grDevices"
 [7] "package:utils"     "package:datasets"  "package:methods"   "Autoloads"         "package:base"     
> library(RPushbullet)
Attaching RPushbullet version 0.2.0.2.
Reading ~/.rpushbullet.json
> file <- testfile(count(TRUE))
>         str(fromJSON(pbPost(type="file", url=file, recipients = 1)[[1]]))
List of 18
 $ active                   : logi TRUE
 $ iden                     : chr "ujCInU"
 $ created                  : num 1.49e+09
 $ modified                 : num 1.49e+09
 $ type                     : chr "file"
 $ dismissed                : logi FALSE
 $ direction                : chr "self"
 $ sender_iden              : chr "ujCG"
 $ sender_email             : chr "wenchel@gmail.com"
 $ sender_email_normalized  : chr "wenchel@gmail.com"
 $ sender_name              : chr "Seth Wenchel"
 $ receiver_iden            : chr "ujCI"
 $ receiver_email           : chr "wenchel@gmail.com"
 $ receiver_email_normalized: chr "wenchel@gmail.com"
 $ target_device_iden       : chr "ujC"
 $ file_name                : chr "test_file_for_post_13.txt"
 $ file_type                : chr "text/plain; charset=utf-8"
 $ file_url                 : chr "https://dl2.pushbulletusercontent.com/8992GiqW/C:%5CUsers%5Cssw26%5CAppData%5CLocal%5CTemp%5CRtmpWqvRMM"| __truncated__
@eddelbuettel
Owner

Put your OR thinking cap and imagine different scenarios from the ones you tested, and ensure those pass.

You for sure broke the setup I have.

@eddelbuettel
Owner
eddelbuettel commented Feb 1, 2017 edited

We are in no rush. RPushbullet on CRAN works.

But rolling back basically all post 0.2.0 changes if we don't sort this out is a clear option.

@restonslacker
Contributor

when you get a chance, would you please test https://github.com/restonslacker/rpushbullet/tree/test/RJSONIO? I want to see if this is jsonlite versus RJSONIO or if both exhibit the behavior you are seeing.

@eddelbuettel
Owner

On train to work so can't but had same thought. I will start from 0.2.0 and check test by test what worked. Or not.

@restonslacker restonslacker deleted the restonslacker:feature/setup branch Feb 1, 2017
@restonslacker
Contributor

In trying to be methodical while working through this, I believe that the intended major changes since the previous CRAN release are:

  • Change from command line curl to CRAN package curl
  • Change from RSJONIO to jsonlite
  • Add function pbSetup()
  • Change simpleTests to remove no longer supported 'Address' push tests and add two tests whose intention is check error handling in the code

Do you concur?

The test/RJSONIO branch starts from e6d967b and removes all references to jsonlite from NAMESPACE and DESCRIPTION and makes updates in the code to support differences in the two JSON packages. It does not roll back to 3d8222a and work forward from there.

Also, as the intention was to see if there was an issue with jsonlite versus RJSONIO, changes in test/RJSONIO were made for short-term expediency vice clarity and changes would likely be advisable if this appears to solve the previously noted issues. E.g. there is probably a cleaner way to write the following block:

ind <- vapply(pdgd$devices, function(x){x$active & x$pushable},TRUE)*(1:length(pdgd$devices))
ind <- ind[ind>0]
@eddelbuettel
Owner

I like the first part of what you wrote and concur with the bullet points. They also show a rank order of importance. Having the curl package is good. Having jsonlite is probably better in the long run but not urgent. Adding pbSetup is meh, and updating tests is "sure why not".

I do not understand what the second half your message is trying to say. But I have other pressing priorities now (spelled "work") so I leave it that.

I suggest we take deep breath, go for walk, let the currently fscked repo sit and start from the basics.

Once I have time (ie not now) I may try to test things with the shipped 0.2.0.

@eddelbuettel
Owner

Time for me to eat some crow. I just put 0.2.0 onto another machine ... and the of course the address test fails (expected) but the two tests after the if (hasDevices) { ... fail there too. So no new regression.

That is good. Less good is that we have shitty tests. I need to reorganize a little and maybe set warnings to be errors to catch this sooner.

@eddelbuettel
Owner

Minor progress, from basic down-to-the-wire debugging. First test fails because the API it no longer wants -d device_iden="..." but rather -d target_device_iden="...". Easy to fix.

@restonslacker
Contributor

Good catch. I will wait for you to reorganize the tests and make other changes, before I do any more development, unless there is something you explicitly ask me to look at, so that we aren't duplicating efforts.

@eddelbuettel
Owner

Thanks, good approach. Else we trample on each.

So after backtracking to 0.2.0 and realizing I was in error, I managed to make two small steps forward. On two different machines I now have to different resource files, one with just one device and one with several devices as before. This all seems to work. So far, so good.

But now that "more works" I notice that I die on your suggested new failing tests. I think, on balance, I am not a fan. I just reverted them to warning() instead, it is still not all that smooth.

Maybe we "park" them behind another until the day we switch to real unit testing with better wrappers? I mostly use RUnit and it catches those better.

Otherwise, getting there.... slowly but surely,

@restonslacker
Contributor

I'm fine with parking those tests for now. I put them in to try and make sure that errors were "getting noticed" by the code, but if the tests themselves don't work correctly, then they aren't very useful or helpful.

@eddelbuettel
Owner

The idea is totally right. I kinda like my little hack with the counter so that it gets to the expected '14' at the end. Maybe we "just" check for the expected error code (anything but success ?).

We could also add a simple function taking res and counting 'good'. We'd be reinventing unit test runners, but we have a hard problem with no public shareable API key so some fudges are required.

So with that: what do you think about aggregating results from the tests to summarize at the end?

@restonslacker
Contributor

I once heard a speaker say, "if you keep reinventing the wheel, eventually you'll make one that's round." ;)

I don't have a strong opinion about the best way to proceed with the tests.

@eddelbuettel
Owner

Ok, taking recourse to executive override as maintainer: I will change the test script to 'hide' the breaking tests behind a second env var. Ok with you? Then if you set both, you get both. If you just set the first you skip. If you have neither (Travis, CRAN, default) nothing tests.

Thoughts?

@restonslacker
Contributor
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment