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

Failing CRAN checks - no deadline yet #174

Closed
nicklucius opened this issue May 25, 2019 · 20 comments
Closed

Failing CRAN checks - no deadline yet #174

nicklucius opened this issue May 25, 2019 · 20 comments

Comments

@nicklucius
Copy link
Contributor

https://cran.r-project.org/web/checks/check_results_RSocrata.html

@nicklucius
Copy link
Contributor Author

@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.

@nicklucius
Copy link
Contributor Author

We received a deadline: June 8.

@nicklucius
Copy link
Contributor Author

@geneorama - did you get the same result regarding the invalid credentials?

@geneorama
Copy link
Member

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.

@geneorama
Copy link
Member

geneorama commented Jun 1, 2019

I emailed Socrata support about creating a dataset that requires authentication for testing.

@geneorama
Copy link
Member

@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.

@nicklucius
Copy link
Contributor Author

nicklucius commented Jun 2, 2019

@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

@nicklucius
Copy link
Contributor Author

@geneorama and @tomschenkjr - we will no longer have access to the [email protected] 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?

@geneorama
Copy link
Member

@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?

@tomschenkjr
Copy link
Contributor

@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.

@levyj
Copy link
Member

levyj commented Jun 6, 2019

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.

@geneorama
Copy link
Member

@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.

@tomschenkjr
Copy link
Contributor

That's an even better point, all tests need to run on CRAN servers.

@geneorama
Copy link
Member

@tomschenkjr thanks! I thought I was missing something.

I updated the Socrata ticket.

@geneorama
Copy link
Member

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

@nicklucius
Copy link
Contributor Author

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.

@tomschenkjr
Copy link
Contributor

tomschenkjr commented Jun 21, 2019 via email

@nicklucius
Copy link
Contributor Author

@tomschenkjr that's great, thanks for the heads up.

@geneorama
Copy link
Member

@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)
#'}

@nicklucius
Copy link
Contributor Author

I added a new instruction in the wiki for manually running skip tests (See # 4): https://github.com/Chicago/RSocrata/wiki/Submitting-to-CRAN

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

No branches or pull requests

4 participants