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

add tests #3

Closed
schochastics opened this issue Sep 22, 2023 · 17 comments
Closed

add tests #3

schochastics opened this issue Sep 22, 2023 · 17 comments
Assignees
Labels

Comments

@schochastics
Copy link
Member

No description provided.

schochastics added a commit that referenced this issue Sep 22, 2023
@chainsawriot
Copy link
Collaborator

crazy things (CRAN won't like them)

  • ada_url_parse("https://中国移动.中国")
  • ada_url_parse("https://www.google.co.jp/search?q=ドイツ")

@schochastics
Copy link
Member Author

Thank god skip_on_cran() exists

@chainsawriot
Copy link
Collaborator

It is slightly more complicated than that. CRAN doesn't allow R scripts with non-ASCII characters (unless with compelling reasons). Therefore, one has to do something like this (which is kind-of okay, if there are only a few tests):

https://github.com/ropensci/readODS/blob/7548c3ab8778efc2d6058fc56ca84920b2e3a92f/tests/testthat/test_xml_fuzz.R#L2

Another way is to group all non-ASCII tests in one file and then .Rbuildignore it. But that would means only the coverage test would test them. Not a good thing.

@chainsawriot
Copy link
Collaborator

I can help with converting the current tests to that form.

@schochastics
Copy link
Member Author

schochastics commented Sep 22, 2023

@chainsawriot Sure happy if you can do that

\Edit: wait, you already did that, didnt you?

@schochastics schochastics added 0.1.0 and removed 0.1.0 labels Sep 22, 2023
@chainsawriot
Copy link
Collaborator

Yes. #10 But I am in the test department, so I can add more tests.

@chainsawriot
Copy link
Collaborator

can't self assigned, but consider this as self assigned.

@chainsawriot
Copy link
Collaborator

chainsawriot commented Sep 23, 2023

  • Improve header hygiene
  • Corner cases

chainsawriot added a commit to chainsawriot/adaR that referenced this issue Sep 23, 2023
* So that we can update ada independently

Freshly updated: https://github.com/ada-url/ada/releases/tag/v2.6.8

* Improve header hygiene gesistsa#3
schochastics pushed a commit that referenced this issue Sep 23, 2023
* Separate our code from ada ref #3

* So that we can update ada independently

Freshly updated: https://github.com/ada-url/ada/releases/tag/v2.6.8

* Improve header hygiene #3

* Rename hpp to h

I've forgotten the CRAN check god doesn't like hpp in `src/`, sorry.
@chainsawriot
Copy link
Collaborator

chainsawriot commented Sep 23, 2023

  • covignore src/ada

@chainsawriot
Copy link
Collaborator

Thanks to utils::URLdecode(), should we fix this? @schochastics

  require(adaR)
#> Loading required package: adaR
  corners <- c(NA, NULL, "", "a", 1)
  testthat::expect_error(x <- ada_url_parse(corners), NA)
  testthat::expect_error(y <- ada_url_parse(corners, decode = FALSE), NA)
  x$host
#> [1] "NA" "NA" "NA" "NA"
  y$host
#> [1] NA NA NA NA

Created on 2023-09-24 with reprex v2.0.2

@schochastics
Copy link
Member Author

Let me deal with #18 first because I think this could make urldecode superfluous. Not sure yet, but will report back

@schochastics
Copy link
Member Author

@chainsawriot I think this needs to be fixed. What I did in #20 doesnt help with URLdecoding afais. You wanna fix it together with the tests?

@chainsawriot
Copy link
Collaborator

Now it's in a TDD (Test-driven development) mode

urls <- rep("http://www.google.com", 3)
adaR::ada_has_credentials(urls) ## ERROR
#> Error in vapply(url, function(x) vapply(url, function(x) Rcpp_ada_has_credentials(x, : values must be length 1,
#>  but FUN(X[[1]]) result is length 3
adaR::ada_has_empty_hostname(urls)
#> http://www.google.com http://www.google.com http://www.google.com 
#>                 FALSE                 FALSE                 FALSE

urls2 <- c(urls, "abc")
adaR::ada_has_empty_hostname(urls2) ## should this be NA?
#> Error in eval(expr, envir, enclos): input is not a valid url

Created on 2023-09-24 with reprex v2.0.2

@schochastics

  1. ada_has_credentials can't vectorize; this line looks wrong.

https://github.com/schochastics/adaR/blob/625ef066b13cd360e39c3c178a4f84e7657b96c6/R/has.R#L9

  1. If has_* functions are vectorized, should they not raise errors with invalid URLs, e.g. give NA_logical_?

I will fix 1; let me know your thought on 2. Thank you very much!

chainsawriot added a commit to chainsawriot/adaR that referenced this issue Sep 24, 2023
@schochastics
Copy link
Member Author

@chainsawriot yes they should probably give NA_logical_. You wanna do that?

@chainsawriot
Copy link
Collaborator

@chainsawriot yes they should probably give NA_logical_. You wanna do that?

Yeah.

This was referenced Sep 24, 2023
@chainsawriot
Copy link
Collaborator

chainsawriot commented Sep 25, 2023

- [ ] Standard Mozilla psl tests

The Mozilla test suite is not like what we are doing.

@chainsawriot
Copy link
Collaborator

chainsawriot commented Sep 25, 2023

  • psl corners

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants