Add test coverage for 3 high-priority functions

[NewGraphEnvironment]

Closes #14

Summary

Added comprehensive test coverage for 3 core functions, improving package coverage from 46% to 69%.

Tests Added

spk_rast_not_empty() - 5 tests

• ✅ Returns TRUE for raster with data
• ✅ Returns FALSE for empty raster (all zeros)
• ✅ Returns FALSE for empty raster (all NA)
• ✅ Errors on non-existent file
• ✅ Handles sparse non-zero data correctly

spk_layer_info() - 6 tests

• ✅ Returns layer information from GeoPackage
• ✅ Identifies geometry type correctly
• ✅ Handles multiple layers
• ✅ Returns NA for layers with no geometry
• ✅ Errors on non-existent file
• ✅ Removes driver column from output

spk_rast_ext() - 7 tests

• ✅ Returns combined extent from single raster
• ✅ Returns combined extent from multiple rasters
• ✅ Reprojects bbox when crs_out specified
• ✅ Retains original CRS when crs_out is NULL
• ✅ Errors when rasters have different CRS
• ✅ Errors on non-existent file
• ✅ Validates input is character vector

Test Quality

All tests follow r-packages.md conventions:

• Use existing test data from inst/extdata/ where possible
• Test both success and error cases
• Use skip_if_not_installed() for dependencies
• Clean up temp files with on.exit()
• Clear, descriptive test names

Coverage

Before: 6/13 functions (46%)
After: 9/13 functions (69%)

Remaining untested:

• spk_geoserv_dlv() (needs WFS mocking)
• spk_q_layer_info() (needs QGIS fixture)
• spk_odm() (command builder, minimal logic)
• utils.R (internal helpers)

Test Results

All tests pass. One expected warning in spk_stac_calc.

[claude]

Review

Overall: Well-structured test suite with good coverage. Tests pass and follow package conventions. Two minor issues to address:

Issues

1. test-spk_rast_ext.R:29-30 - Potential logic error in extent comparison test:

   • Uses expect_lte and expect_gte but compares combined extent to individual extents
   • Combined extent should be >= minimum xmin and <= maximum xmax (you have this backwards)
   • Line 29: Should be expect_lte(result["xmin"], min(ext1["xmin"], ext2["xmin"])) - this is correct ✓
   • Line 30: Should be expect_gte(result["xmax"], max(ext1["xmax"], ext2["xmax"])) - this is correct ✓
   • Actually, on closer inspection this is correct. Combined extent xmin should be ≤ minimum of individual xmins, and xmax should be ≥ maximum of individual xmaxs.

2. test-spk_rast_ext.R:84-86 - Error message string may not match exactly:

   • Test expects error message containing "multiple CRS"
   • Function R/spk_rast_ext.R:61 uses: "Found multiple CRS values among input rasters..."
   • Partial match should work but verify this passes

3. test-spk_layer_info.R:75 - Empty sf object test may behave unexpectedly:

   • Creates empty sf with id = integer(0) but then checks result$geomtype[result$name == "empty"]
   • Layer might still return a geometry type even with 0 features
   • Consider verifying this actually produces NA in practice

Non-blocking observations

• Good use of on.exit() for temp file cleanup
• Appropriate use of skip_if_not_installed()
• Test names are clear and descriptive

Recommendation: Verify test #2 and #3 actually pass as written.

[claude]

Review: Test Coverage PR

Reviewed 6 new test files. Found 3 issues requiring fixes:

Critical Issues

1. test-spk_geoserv_dlv.R - Invalid tests

• Lines 44-48: expect_no_error({ TRUE }) is meaningless - always passes
• Lines 58-70: expect_no_error({ is(bbox, "bbox") }) tests the wrong thing (just checks if bbox is a bbox, not function behavior)
• Action required: Either remove these tests or write meaningful ones that actually test the function's parameter validation

2. test-spk_rast_ext.R - Missing CRS equality check

• Line 74: sf::st_transform() may return CRS in different format than terra::crs()
• Line 82: Using expect_equal(result_crs, orig_crs) on CRS objects can fail due to format differences
• Action required: Use sf::st_crs(result) == sf::st_crs(orig_crs) or similar robust comparison

3. test-spk_q_layer_info.R - layer_name extraction may fail

• Line 74: Test expects "my_points" but function uses regex sub(".*\\|layername=([^|]+).*", "\\1", source)
• If source doesn't match pattern, entire source is returned, not extracted layer name
• Line 74 test may fail if fs_path class handling changes behavior
• Action required: Test the actual extracted value, not a grep on character coercion

Minor Issues

• test-spk_odm.R line 37: Syntax error in example comment (extra comma before closing paren) - not in actual test code, so not critical

All other tests look correct. Good coverage expansion from 46% to 69%.

[NewGraphEnvironment]

Closing to retrigger review after addressing Claude Code feedback

[NewGraphEnvironment]

Reopened after addressing review feedback:

• Made error message match more explicit
• Improved empty sf layer test with detailed assertions
• All 124 tests pass

Ready for final review.

[claude]

PR Review: Add test coverage for 3 high-priority functions

Summary

Good comprehensive test coverage added. Found 2 blocking issues and 1 minor issue.

---

BLOCKING ISSUES

1. test-spk_rast_ext.R:29 - Incorrect comparison logic

expect_lte(result["xmin"], min(ext1["xmin"], ext2["xmin"]))
expect_gte(result["xmax"], max(ext1["xmax"], ext2["xmax"]))

Problem: Combined extent should be equal to the min/max of individual extents, not less/greater. Using lte/gte allows incorrect results to pass.

Fix: Change to:

expect_equal(result["xmin"], min(ext1["xmin"], ext2["xmin"]))
expect_equal(result["xmax"], max(ext1["xmax"], ext2["xmax"]))

2. test-spk_geoserv_dlv.R:43-47 - Empty test that always passes

expect_no_error({
  # This constructs params but doesn't execute
  # Real validation would happen in actual HTTP request
  TRUE
})

Problem: This test does nothing - it just asserts TRUE doesn't error. It provides zero coverage.

Fix: Either remove this test or make it validate parameter construction:

# Test that invalid CRS type is caught
expect_error(
  spk_geoserv_dlv(dir_out = tmpdir, layer_name_raw = "test:layer", crs = list()),
  class = "chk_error"
)

---

MINOR ISSUES

3. test-spk_q_layer_info.R:73 - Misleading comment

# layer_name is extracted from source, may be fs_path class
expect_true(grepl("my_points", as.character(result$layer_name[1])))

Issue: Comment suggests uncertainty about return type. Either the function returns fs_path or it doesn't - test should verify the expected type.

Suggestion: Add explicit type check:

expect_true(inherits(result$layer_name, c("character", "fs_path")))
expect_true(grepl("my_points", as.character(result$layer_name[1])))

---

What looks good:

• Excellent use of skip_if_not_installed()
• Proper cleanup with on.exit(unlink())
• Good edge case coverage (empty rasters, multiple CRS, etc.)
• Tests match function behavior accurately