Closed nicklucius closed 5 years ago
@geneorama I fixed the errors due to Socrata changing the column order, but it looks like the login used to test write.socrata()
has been modified. I am getting an invalid credentials error in my tests. Could you check that you get the same thing? If so, we'll need to take this up with Socrata.
We received a deadline: June 8.
@geneorama - did you get the same result regarding the invalid credentials?
I think that the credentials we're using in our test have changed. read.socrata
isn't working in the test, but it's working when I put my own credentials in.
I emailed Socrata support about creating a dataset that requires authentication for testing.
@nicklucius did you see problems with other tests?
If this is the only one I think it would be best if we temporarily removed the test and created an issue to add it back in.
@geneorama - yes I got other errors, but they are all fixed with the commit referenced above. Now I am only receiving the following authentication-related errors:
────────────────────────────────────────────────────────
test-all.R:463: error: read Socrata CSV that requires a login
Forbidden (HTTP 403).
1: read.socrata(url = privateResourceToReadCsvUrl, email = socrataEmail,
password = socrataPassword) at /Users/lucius/OneDrive - City of Chicago/Repos/RSocrata/tests/testthat/test-all.R:463
2: getResponse(validUrl, email, password) at /Users/lucius/OneDrive - City of Chicago/Repos/RSocrata/R/RSocrata.R:345
3: httr::stop_for_status(response) at /Users/lucius/OneDrive - City of Chicago/Repos/RSocrata/R/RSocrata.R:227
test-all.R:473: error: read Socrata JSON that requires a login
Forbidden (HTTP 403).
1: read.socrata(url = privateResourceToReadJsonUrl, email = socrataEmail,
password = socrataPassword) at /Users/lucius/OneDrive - City of Chicago/Repos/RSocrata/tests/testthat/test-all.R:473
2: getResponse(validUrl, email, password) at /Users/lucius/OneDrive - City of Chicago/Repos/RSocrata/R/RSocrata.R:345
3: httr::stop_for_status(response) at /Users/lucius/OneDrive - City of Chicago/Repos/RSocrata/R/RSocrata.R:227
────────────────────────────────────────────────────────
✖ | 0 2 | write Socrata datasets [0.2 s]
────────────────────────────────────────────────────────
test-all.R:493: failure: add a row to a dataset
res$status_code not equal to 200.
1/1 mismatches
[1] 403 - 200 == 203
test-all.R:510: failure: fully replace a dataset
res$status_code not equal to 200.
1/1 mismatches
[1] 403 - 200 == 203
@geneorama and @tomschenkjr - we will no longer have access to the mark.silverberg+soda.demo@socrata.com account that was used to test read.socrata
in part and write.socrata
in full. Also, Socrata is not going to provide a replacement.
I think that means we'll have to implement #45 in order to maintain unit test coverage. As a reminder, the deadline to remain on CRAN is this Saturday.
Thoughts?
@nicklucius I think all the test failures are stemming from the authentication problem.
@tomschenkjr Socrata suggested that we use encrypted variables. That would work on GitHub, but I can't tell if it would be supported in CRAN. Do you know?
@nicklucius - I think moving to CoC would be fairly easy for the read/write. You'll want to talk to @levyj whether he'd want that on the portal and to make sure the user only can see that particular data set.
@geneorama - Yeah, it's been suggested before. However, I'm hesitant because using encrypted or hidden variables doesn't allow local testing (e.g., running unit tests on your laptop) because it won't know how to handle it. Because it's a very limited risk (reading/writing to only one data set), opted to forgo encrypted or environmental variables.
I don't think all of the current failures are due to authentication. Some of the tests failing don't have a login requirement.
We have talked about it a bit. I would want to go through the formality of getting higher-level approval but I am fine with it and would not expect that to be a barrier.
@tomschenkjr issue #137 gives me a little more insight about the drawbacks; losing the ability to test locally would be bad.
However, I don't think it would even solve the problem of failing CRAN tests. They're not running Travis to do the tests.
Also, I just fixed the tests, and I realized we'd have another problem. The examples in our documentation rely on the credentials as well.
That's an even better point, all tests need to run on CRAN servers.
@tomschenkjr thanks! I thought I was missing something.
I updated the Socrata ticket.
To recap:
We're asking for an extension (@nicklucius sent / sending email)
We're seeking a coc account to use in place of the Silverberg account
We have an extended deadline which is due tomorrow, 6/22. We are unable to acquire an email and data portal account for testing RSocrata at this time. I believe this means our best option is to comment out the automated tests that require authentication, and then add to the CRAN submission instructions on the wiki so that we remember to manually run these tests locally before submitting to CRAN.
If anyone (@geneorama and @tomschenkjr, looking in your direction) has thoughts or a better way to handle this, please chime in.
You can use the skip()
function in the unit testing file to skip the test
on CRAN, but otherwise keep the test (and provide instructions that you
mentioned).
On Fri, Jun 21, 2019 at 9:13 AM Nick Lucius notifications@github.com wrote:
We have an extended deadline which is due tomorrow, 6/22. We are unable to acquire an email and data portal account for testing RSocrata at this time. I believe this means our best option is to comment out the automated tests that require authentication, and then add to the CRAN submission instructions https://github.com/Chicago/RSocrata/wiki/Submitting-to-CRAN on the wiki so that we remember to manually run these tests locally before submitting to CRAN.
If anyone (@geneorama https://github.com/geneorama and @tomschenkjr https://github.com/tomschenkjr, looking in your direction) has thoughts or a better way to handle this, please chime in.
— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/Chicago/RSocrata/issues/174?email_source=notifications&email_token=AAMQFUMRVECLQN5RONGWHALP3TO2PA5CNFSM4HPTS7IKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODYISTWY#issuecomment-504441307, or mute the thread https://github.com/notifications/unsubscribe-auth/AAMQFUNP4O7VSR752LBL57TP3TO2PANCNFSM4HPTS7IA .
-- Tom Schenk Jr. tomschenkjr@gmail.com @tomschenkjr tomschenkjr.net
@tomschenkjr that's great, thanks for the heads up.
@nicklucius (and @tomschenkjr )looks like we can skip the examples in the documentation with a don't run command: https://stackoverflow.com/questions/12038160/how-to-not-run-an-example-using-roxygen2
Use \dontrun{}
'@examples
'\dontrun{
'geocode("3817 Spruce St, Philadelphia, PA 19104")
'geocode("Philadelphia, PA")
'dat <- data.frame(value=runif(3),address=c("3817 Spruce St, Philadelphia, PA 19104","Philadelphia, PA","Neverneverland"))
'geocode(dat)
'}
I added a new instruction in the wiki for manually running skip tests (See # 4): https://github.com/Chicago/RSocrata/wiki/Submitting-to-CRAN
https://cran.r-project.org/web/checks/check_results_RSocrata.html