start small refactoring #46

maelle · 2022-03-02T07:34:54Z

Mostly simplifying control flow.

In tests I use skip_if_offline() which with the latest testthat version also does skip_on_cran(). I can't cache all responses as some of them are not done with httr (but with a sf function IIRC). With the skipping, the tests don't fail when offline. (I've turned of wifi).

TODOS

Re-read test changes,
Add tests for edge cases,
Try using less fixtures or small ones,
Squash and merge, ensuring Anna is an author.

R/layers.R

codecov-commenter · 2022-03-02T08:41:40Z

Codecov Report

Merging #46 (b63d4a5) into master (fcab4f6) will increase coverage by 0.35%.
The diff coverage is 89.87%.

@@            Coverage Diff             @@
##           master      #46      +/-   ##
==========================================
+ Coverage   84.86%   85.21%   +0.35%     
==========================================
  Files           4        5       +1     
  Lines         218      230      +12     
==========================================
+ Hits          185      196      +11     
- Misses         33       34       +1

Impacted Files	Coverage Δ
R/layers.R	`82.19% <85.71%> (+2.19%)`	⬆️
R/info.R	`87.80% <90.00%> (-2.20%)`	⬇️
R/client.R	`86.27% <100.00%> (+0.56%)`	⬆️
R/emodnet_wfs.R	`100.00% <100.00%> (ø)`
R/layer_attributes.R	`85.00% <100.00%> (-0.94%)`	⬇️

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 4afc00b...b63d4a5. Read the comment docs.

R/layers.R

maelle · 2022-03-02T13:17:03Z

tests/testthat/_snaps/layers.md

+
+    Assertion on 'wfs' failed: Must be an R6 class, not character.
+
+# emodnet_get_layers errors well when bad layer


probably not the best error message, actually.

R/client.R

R/info.R

maelle · 2022-03-03T07:35:08Z

tests/testthat/helper.R

+    httptest::with_mock_dir(testthat::test_path(file.path("fixtures", name)), ...)
+}
+
+create_biology_wfs <- function() {


this way we re-use this one in most tests, with a single fixture :-)

maelle · 2022-03-03T07:35:19Z

tests/testthat/setup.R

@@ -0,0 +1 @@
+pre_test_options <- options(usethis.quiet = TRUE)


this is how it's done in usethis itself

maelle · 2022-03-03T07:35:58Z

tests/testthat/test-client.R

+    wfs <- create_biology_wfs()
+    expect_equal(class(wfs), c("WFSClient", "OWSClient", "OGCAbstractObject", "R6"))
+    expect_equal(wfs$getUrl(), "http://geo.vliz.be/geoserver/Emodnetbio/wfs")
+})

 test_that("Specified connection works", {


@annakrystalli what was the difference between default and specified connection? I'm not sure I got it so I might have badly altered the test.

maelle · 2022-03-03T07:36:22Z

tests/testthat/test-filter.R

-
-   expect_equal(or_filter_sf$country %>% unique(), c("Bulgaria", "Baltic Sea"))
-
+    skip_if_offline()


@annakrystalli simpler filters because I used smaller layers.

annakrystalli · 2022-03-11T11:32:38Z

tests/testthat/test-layers.R

-  expect_length(l_crs, 1)
-  expect_equal(3857, l_crs)
+test_that("crs transform works from wfs object", {
+    skip_if_offline()


Question: Do we need to skip if offline if we are going to use cached results through httptest?

annakrystalli

I've only one question regarding skip if offline below.

Once that's resolved, I'm happy to merge

annakrystalli · 2022-03-11T11:34:26Z

Ooops, some merge conflicts too. Let me know if you want me to tackle them @maelle

annakrystalli · 2022-03-14T10:03:17Z

Still trying to figure out what the problem is with failing tests. The annoying thing is that the code works when run outside of the testing framework:

library(EMODnetWFS)
library(testthat)

create_biology_wfs <- function() {
        emodnet_init_wfs_client(service = "biology")
}

wfs <- create_biology_wfs()
#> Loading ISO 19139 XML schemas...
#> Loading ISO 19115 codelists...
#> Loading IANA mime types...
#> No encoding supplied: defaulting to UTF-8.
#> ✓ WFS client created succesfully
#> ℹ Service: 'http://geo.vliz.be/geoserver/Emodnetbio/wfs'
#> ℹ Version: '2.0.0'
simple_filter_sf <- emodnet_get_layers(
    wfs = wfs,
    layers = "mediseh_cymodocea_pnt",
    cql_filter = "country='Grecia'",
    reduce_layers = TRUE
)
simple_filter_sf
#> Simple feature collection with 159 features and 3 fields
#> Geometry type: POINT
#> Dimension:     XY
#> Bounding box:  xmin: 20.01482 ymin: 35.19033 xmax: 28.20996 ymax: 40.95
#> Geodetic CRS:  WGS 84
#> First 10 features:
#>                       gml_id id country                  the_geom
#> 1  mediseh_cymodocea_pnt.981  0  Grecia POINT (24.88624 37.43593)
#> 2  mediseh_cymodocea_pnt.982  0  Grecia POINT (24.66667 36.96667)
#> 3  mediseh_cymodocea_pnt.983  0  Grecia POINT (23.91667 37.71667)
#> 4  mediseh_cymodocea_pnt.984  0  Grecia POINT (24.43333 36.71667)
#> 5  mediseh_cymodocea_pnt.985  0  Grecia         POINT (26.2 39.2)
#> 6  mediseh_cymodocea_pnt.986  0  Grecia POINT (26.48333 39.08333)
#> 7  mediseh_cymodocea_pnt.987  0  Grecia POINT (24.00725 37.92523)
#> 8  mediseh_cymodocea_pnt.988  0  Grecia        POINT (24.5 40.95)
#> 9  mediseh_cymodocea_pnt.989  0  Grecia POINT (24.56667 40.91667)
#> 10 mediseh_cymodocea_pnt.990  0  Grecia  POINT (24.3176 40.81667)
expect_equal(unique(simple_filter_sf$country), 'Grecia')

^{Created on 2022-03-14 by the reprex package (v2.0.1)}

Yet during testing they fail.

==> Testing R file using 'testthat'

ℹ Loading EMODnetWFS
Loading ISO 19139 XML schemas...
Loading ISO 19115 codelists...
Loading IANA mime types...
No encoding supplied: defaulting to UTF-8.

══ Testing test-filter.R ═══════════════════════════════════════════════════════

[ FAIL 0 | WARN 0 | SKIP 0 | PASS 0 ]
[ FAIL 0 | WARN 1 | SKIP 0 | PASS 0 ]
[ FAIL 1 | WARN 1 | SKIP 0 | PASS 0 ]
[ FAIL 1 | WARN 2 | SKIP 0 | PASS 0 ]
[ FAIL 2 | WARN 2 | SKIP 0 | PASS 0 ]
[ FAIL 3 | WARN 2 | SKIP 0 | PASS 0 ]
[ FAIL 4 | WARN 2 | SKIP 0 | PASS 0 ]
[ FAIL 4 | WARN 3 | SKIP 0 | PASS 0 ]
[ FAIL 4 | WARN 4 | SKIP 0 | PASS 0 ]
[ FAIL 4 | WARN 4 | SKIP 0 | PASS 1 ]
[ FAIL 5 | WARN 4 | SKIP 0 | PASS 1 ]

── Warning (test-filter.R:4:5): categorical filters work ───────────────────────
Download of layer 'mediseh_cymodocea_pnt' failed: Error: GET http://geo.vliz.be/geoserver/Emodnetbio/wfs?service=WFS&version=2.0.0&typeNames=Emodnetbio:mediseh_cymodocea_pnt&cql_filter=country='Grecia'&request=GetFeature (geo.vliz.be/geoserver/Emodnetbio/wfs-55e352.json)

Backtrace:
  1. with_mock_dir(...)
       at test-filter.R:4:4
  9. EMODnetWFS::emodnet_get_layers(...)
       at test-filter.R:5:8
 12. purrr::map2(...)
 13. EMODnetWFS .f(.x[[1L]], .y[[1L]], ...)
 14. EMODnetWFS::ews_get_layer(.x, wfs, cql_filter = .y)
 15. base::tryCatch(...)
       at EMODnetWFS/R/layers.R:191:8
 16. base tryCatchList(expr, classes, parentenv, handlers)
 17. base tryCatchOne(expr, names, parentenv, handlers[[1L]])
 18. value[[3L]](cond)
 19. usethis::ui_warn("Download of layer {usethis::ui_value(x)} failed: {usethis::ui_field(e)}")
       at EMODnetWFS/R/layers.R:195:16

── Failure (test-filter.R:4:5): categorical filters work ───────────────────────
unique(simple_filter_sf$country) (`actual`) not equal to "Grecia" (`expected`).

`actual` is NULL
`expected` is a character vector ('Grecia')
Backtrace:
 1. with_mock_dir(...)
      at test-filter.R:4:4
 9. testthat::expect_equal(unique(simple_filter_sf$country), "Grecia")
      at test-filter.R:11:8

── Warning (test-filter.R:15:5): categorical filters work ──────────────────────
Download of layer 'mediseh_cymodocea_pnt' failed: Error: GET http://geo.vliz.be/geoserver/Emodnetbio/wfs?service=WFS&version=2.0.0&typeNames=Emodnetbio:mediseh_cymodocea_pnt&cql_filter=country='Francia'%20OR%20country=='Grecia'&request=GetFeature (geo.vliz.be/geoserver/Emodnetbio/wfs-ddf2c0.json)

Backtrace:
  1. with_mock_dir(...)
       at test-filter.R:15:4
  9. EMODnetWFS::emodnet_get_layers(...)
       at test-filter.R:16:8
 12. purrr::map2(...)
 13. EMODnetWFS .f(.x[[1L]], .y[[1L]], ...)
 14. EMODnetWFS::ews_get_layer(.x, wfs, cql_filter = .y)
 15. base::tryCatch(...)
       at EMODnetWFS/R/layers.R:191:8
 16. base tryCatchList(expr, classes, parentenv, handlers)
 17. base tryCatchOne(expr, names, parentenv, handlers[[1L]])
 18. value[[3L]](cond)
 19. usethis::ui_warn("Download of layer {usethis::ui_value(x)} failed: {usethis::ui_field(e)}")
       at EMODnetWFS/R/layers.R:195:16

── Failure (test-filter.R:15:5): categorical filters work ──────────────────────
unique(or_filter_sf$country) (`actual`) not equal to c("Francia", "Grecia") (`expected`).

`actual` is NULL
`expected` is a character vector ('Francia', 'Grecia')
Backtrace:
 1. with_mock_dir(...)
      at test-filter.R:15:4
 9. testthat::expect_equal(...)
      at test-filter.R:22:8

── Failure (test-filter.R:24:5): categorical filters work ──────────────────────
unique(or_filter_sf$country) (`actual`) not equal to c("Francia", "Grecia") (`expected`).

`actual` is NULL
`expected` is a character vector ('Francia', 'Grecia')

── Error (test-filter.R:27:5): categorical filters work ────────────────────────
Error in `curl::curl_fetch_memory(url, handle = handle)`: Timeout was reached: [drive.emodnet-geology.eu] Operation timed out after 10001 milliseconds with 0 out of 0 bytes received
Backtrace:
  1. EMODnetWFS::emodnet_init_wfs_client(service = "geology_seabed_substrate_maps")
       at test-filter.R:27:4
  9. httr::GET(.)
 10. httr:::request_perform(req, hu$handle$handle)
 12. httr:::request_fetch.write_memory(req$output, req$url, handle)
 13. curl::curl_fetch_memory(url, handle = handle)

── Warning (test-filter.R:47:5): numeric filters work ──────────────────────────
Download of layer 'mediseh_posidonia_nodata' failed: Error: GET http://geo.vliz.be/geoserver/Emodnetbio/wfs?service=WFS&version=2.0.0&typeNames=Emodnetbio:mediseh_posidonia_nodata&cql_filter=km%3E400&request=GetFeature (geo.vliz.be/geoserver/Emodnetbio/wfs-01f28e.json)

Backtrace:
  1. with_mock_dir(...)
       at test-filter.R:47:4
  9. EMODnetWFS::emodnet_get_layers(...)
       at test-filter.R:48:8
 12. purrr::map2(...)
 13. EMODnetWFS .f(.x[[1L]], .y[[1L]], ...)
 14. EMODnetWFS::ews_get_layer(.x, wfs, cql_filter = .y)
 15. base::tryCatch(...)
       at EMODnetWFS/R/layers.R:191:8
 16. base tryCatchList(expr, classes, parentenv, handlers)
 17. base tryCatchOne(expr, names, parentenv, handlers[[1L]])
 18. value[[3L]](cond)
 19. usethis::ui_warn("Download of layer {usethis::ui_value(x)} failed: {usethis::ui_field(e)}")
       at EMODnetWFS/R/layers.R:195:16

── Warning (test-filter.R:54:5): numeric filters work ──────────────────────────
no non-missing arguments to min; returning Inf
Backtrace:
 1. testthat::expect_true(min(num_filter_sf$km) > 400)
      at test-filter.R:54:4
 2. testthat::quasi_label(enquo(object), label, arg = "object")
 3. rlang::eval_bare(expr, quo_get_env(quo))

── Error (test-filter.R:57:5): numeric filters work ────────────────────────────
Error in `curl::curl_fetch_memory(url, handle = handle)`: Timeout was reached: [drive.emodnet-geology.eu] Operation timed out after 10003 milliseconds with 0 out of 0 bytes received
Backtrace:
  1. EMODnetWFS::emodnet_init_wfs_client(service = "geology_seabed_substrate_maps")
       at test-filter.R:57:4
  9. httr::GET(.)
 10. httr:::request_perform(req, hu$handle$handle)
 12. httr:::request_fetch.write_memory(req$output, req$url, handle)
 13. curl::curl_fetch_memory(url, handle = handle)


[ FAIL 5 | WARN 4 | SKIP 0 | PASS 1 ]

annakrystalli · 2022-03-14T10:08:50Z

I feel like it is something to do with how the fixtures are set up.

I also noticed something strange in the testthat folder, that there is a fixtures folder within it and then also a tests/testthat/fixtures folder.

fs::dir_tree("tests", 4)
#> /Users/Anna/Documents/workflows/EMODnet/EMODnetWFS/tests
#> ├── testthat
#> │   ├── _snaps
#> │   │   ├── layer_attributes.md
#> │   │   └── layers.md
#> │   ├── fixtures
#> │   │   ├── bathymetry-info
#> │   │   │   └── ows.emodnet-bathymetry.eu
#> │   │   │       └── wfs-8af060.xml
#> │   │   ├── biology-attr2
#> │   │   │   └── geo.vliz.be
#> │   │   │       └── geoserver
#> │   │   ├── biology-attr3
#> │   │   │   └── geo.vliz.be
#> │   │   │       └── geoserver
#> │   │   ├── biology-info
#> │   │   │   └── geo.vliz.be
#> │   │   │       └── geoserver
#> │   │   ├── mediseh_cymodocea_pnt-Francia-Grecia
#> │   │   │   └── geo.vliz.be
#> │   │   │       └── geoserver
#> │   │   ├── mediseh_cymodocea_pnt-Grecia
#> │   │   │   └── geo.vliz.be
#> │   │   │       └── geoserver
#> │   │   └── mediseh_posidonia_nodata
#> │   │       └── geo.vliz.be
#> │   │           └── geoserver
#> │   ├── helper.R
#> │   ├── setup.R
#> │   ├── teardown.R
#> │   ├── test-client.R
#> │   ├── test-filter.R
#> │   ├── test-info.R
#> │   ├── test-layer_attributes.R
#> │   ├── test-layers.R
#> │   ├── testdata
#> │   │   ├── attr_desc.rds
#> │   │   ├── maritime_crs.rds
#> │   │   └── test-data-prep.R
#> │   └── tests
#> │       └── testthat
#> │           └── fixtures
#> │               ├── bathymetry-info
#> │               ├── biology-attr
#> │               ├── biology-attr2
#> │               ├── biology-attr3
#> │               ├── biology-info
#> │               ├── layers-biology
#> │               ├── layers-biology2
#> │               ├── layers-biology3
#> │               ├── layers-biology4
#> │               ├── layers-biology5
#> │               ├── mediseh_cymodocea_pnt-Francia-Grecia
#> │               ├── mediseh_cymodocea_pnt-Grecia
#> │               ├── mediseh_posidonia_nodata
#> │               ├── reduce
#> │               └── wfs-biology
#> └── testthat.R

^{Created on 2022-03-14 by the reprex package (v2.0.1)}

maelle added 2 commits March 2, 2022 08:06

start small refactoring

f673183

some light refactoring

00b1a19

maelle commented Mar 2, 2022

View reviewed changes

R/layers.R Show resolved Hide resolved

maelle commented Mar 2, 2022

View reviewed changes

R/layers.R Show resolved Hide resolved

maelle commented Mar 2, 2022

View reviewed changes

R/layers.R Show resolved Hide resolved

maelle commented Mar 2, 2022

View reviewed changes

R/layers.R Show resolved Hide resolved

maelle commented Mar 2, 2022

View reviewed changes

R/layers.R Show resolved Hide resolved

maelle commented Mar 2, 2022

View reviewed changes

R/layers.R Show resolved Hide resolved

maelle commented Mar 2, 2022

View reviewed changes

R/layers.R Show resolved Hide resolved

maelle added 6 commits March 2, 2022 08:38

ws

4737aa5

ws

adfde1d

mostly ws + more logical order

c81689c

make tests pass for me :-)

6a00406

shut up

a8c0572

fix or new error?

15a6717

annakrystalli reviewed Mar 2, 2022

View reviewed changes

R/layers.R Show resolved Hide resolved

modernize workflow + add run_dont_run cc @annakrystalli

2cf01d5

maelle mentioned this pull request Mar 2, 2022

Edge case for emodnet_get_layer_info() with biology_occurrence_data #60

Closed

Fix #60

79c43ab

maelle commented Mar 2, 2022

View reviewed changes

maelle added 8 commits March 2, 2022 15:37

fix one test yay thanks @annakrystalli

1bf3a1e

better

87bdcc6

httptest here too

bd82d74

ua

efb2be5

tests

a591319

tweaks

4bfc4d6

better

fef8327

less fixtures

8337bc9

maelle commented Mar 3, 2022

View reviewed changes

R/client.R Show resolved Hide resolved

maelle commented Mar 3, 2022

View reviewed changes

R/info.R Show resolved Hide resolved

maelle commented Mar 3, 2022

View reviewed changes

maelle added 2 commits March 3, 2022 09:33

fix check

3d7b4bb

Merge branch 'master' into simpler

9f9da1d

maelle marked this pull request as ready for review March 3, 2022 08:38

maelle mentioned this pull request Mar 11, 2022

Bug to explore #70

Closed

annakrystalli reviewed Mar 11, 2022

View reviewed changes

annakrystalli approved these changes Mar 11, 2022

View reviewed changes

Merge branch 'master' into simpler

5435334

annakrystalli and others added 4 commits March 14, 2022 10:39

add namespace to layer names for GetFeatures

34973c7

mock_dir call fix cc @annakrystalli

d5a2534

update fixtures

b63d4a5

Increment version number to 2.0.1.9000

3f2ee2c

annakrystalli merged commit 35933ba into master Mar 14, 2022

annakrystalli deleted the simpler branch March 14, 2022 12:37

annakrystalli mentioned this pull request Mar 14, 2022

Filtering using PROPERTYNAME in attribute functions is failing #71

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

start small refactoring #46

start small refactoring #46

maelle commented Mar 2, 2022 •

edited

codecov-commenter commented Mar 2, 2022 •

edited

maelle Mar 2, 2022

maelle Mar 3, 2022

maelle Mar 3, 2022

maelle Mar 3, 2022

maelle Mar 3, 2022

annakrystalli Mar 11, 2022

annakrystalli left a comment

annakrystalli commented Mar 11, 2022

annakrystalli commented Mar 14, 2022

annakrystalli commented Mar 14, 2022


		Assertion on 'wfs' failed: Must be an R6 class, not character.

		# emodnet_get_layers errors well when bad layer

		@@ -0,0 +1 @@
		pre_test_options <- options(usethis.quiet = TRUE)


		expect_equal(or_filter_sf$country %>% unique(), c("Bulgaria", "Baltic Sea"))

		skip_if_offline()

start small refactoring #46

start small refactoring #46

Conversation

maelle commented Mar 2, 2022 • edited

codecov-commenter commented Mar 2, 2022 • edited

Codecov Report

maelle Mar 2, 2022

Choose a reason for hiding this comment

maelle Mar 3, 2022

Choose a reason for hiding this comment

maelle Mar 3, 2022

Choose a reason for hiding this comment

maelle Mar 3, 2022

Choose a reason for hiding this comment

maelle Mar 3, 2022

Choose a reason for hiding this comment

annakrystalli Mar 11, 2022

Choose a reason for hiding this comment

annakrystalli left a comment

Choose a reason for hiding this comment

annakrystalli commented Mar 11, 2022

annakrystalli commented Mar 14, 2022

annakrystalli commented Mar 14, 2022

maelle commented Mar 2, 2022 •

edited

codecov-commenter commented Mar 2, 2022 •

edited