Fixing Unit Tests and CI

[leogdion]

No description provided.

[coderabbitai]

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

• 🔍 Trigger a full review

✨ Finishing touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch subrepo-ci

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[claude]

Code Review - PR #155: Fixing Unit Tests and CI

Summary

This PR removes flaky network-dependent integration tests and keeps only tests that validate error handling for invalid URLs. The change improves CI reliability by eliminating external dependencies.

---

✅ Positive Changes

1. Eliminates Flaky Tests: Removing network-dependent tests that hit external URLs (apple.com, httpbin.org) prevents CI failures due to:

   • Network timeouts
   • DNS resolution issues
   • Service downtime
   • Unpredictable response headers

2. Better Documentation: Updated class-level doc comment clarifies that tests "validate core functionality without requiring network access"

3. Focused Test Coverage: Remaining tests verify the actual error handling behavior, which is more valuable than integration tests in a unit test suite

---

⚠️ Issues & Concerns

1. Critically Low Test Coverage

The PR removes 5 out of 7 tests, leaving only 2 tests that cover error scenarios. This significantly reduces test coverage for URLSession extensions:

Lost Coverage:

• ✗ Valid URL with Last-Modified header (removed testFetchLastModifiedWithValidURL)
• ✗ URL without Last-Modified header (removed testFetchLastModifiedReturnsNilGracefully)
• ✗ Successful data fetching (removed testFetchDataWithValidURL)
• ✗ trackLastModified: false parameter (removed testFetchDataWithoutLastModifiedTracking)

Remaining Coverage:

• ✓ Invalid URL returns nil for fetchLastModified
• ✓ Invalid URL throws error for fetchData

Missing Test Scenarios:

• HTTP status code validation (200-299 range check in URLSession.swift:58)
• Non-HTTP responses (URLSession.swift:54-56)
• Last-Modified header extraction (URLSession.swift:68)
• trackLastModified parameter behavior
• RFC2822 date parsing (URLSession.swift:90)

2. Inconsistent Indentation

Lines 48-52 and 57-63 have incorrect indentation (extra spaces):

// Before (incorrect):
    let url = try XCTUnwrap(
      URL(string: "https://this-domain-definitely-does-not-exist-12345.com")
    )

      let lastModified = await URLSession.shared.fetchLastModified(from: url)  // Wrong indent
      XCTAssertNil(lastModified)  // Wrong indent

// Should be:
    let url = try XCTUnwrap(
      URL(string: "https://this-domain-definitely-does-not-exist-12345.com")
    )
    
    let lastModified = await URLSession.shared.fetchLastModified(from: url)
    XCTAssertNil(lastModified)

This should be caught by swift-format when running ./Scripts/lint.sh.

3. Better Testing Approach Needed

Instead of removing tests, consider:

Option A: Mock URLSession (Recommended)

protocol URLSessionProtocol {
  func data(from url: URL) async throws -> (Data, URLResponse)
  func data(for request: URLRequest) async throws -> (Data, URLResponse)
}

extension URLSession: URLSessionProtocol {}

// Then inject mocks in tests for deterministic behavior

Option B: Use URLProtocol for Stubbing

// Register custom URLProtocol to intercept and stub network requests
class MockURLProtocol: URLProtocol {
  static var mockData: [URL: (Data, HTTPURLResponse)] = [:]
  // ... implementation
}

Option C: Separate Integration Tests

• Keep unit tests with mocks in BushelUtilitiesTests
• Move network tests to separate BushelIntegrationTests target (excluded from standard CI)

---

📋 Recommendations

Must Fix:

1. Fix indentation - Run ./Scripts/lint.sh and fix formatting issues
2. Add test coverage - Either:
   • Add mock-based tests to restore coverage for success paths, OR
   • Document in PR description why reduced coverage is acceptable (e.g., "integration tests moved elsewhere")

Should Consider:

3. Document trade-offs - Add PR description explaining:

   • What broke in CI (specific failure logs)
   • Why deletion was chosen over mocking
   • Impact on overall test coverage metrics

4. Verify HTTPURLResponse extension - The removed tests relied on httpResponse.lastModified (line 68). Ensure this extension exists:

   extension HTTPURLResponse {
     var lastModified: Date? {
       guard let lastModifiedString = value(forHTTPHeaderField: "Last-Modified") else {
         return nil
       }
       return Date(rfc2822String: lastModifiedString)
     }
   }

5. Consider timeouts - If keeping these tests, add explicit timeouts:

   try await withTimeout(seconds: 5) {
     _ = try await URLSession.shared.fetchData(from: url)
   }

---

🔒 Security, Performance, Style

• Security: ✅ No concerns (test code only)
• Performance: ✅ Improved (faster test execution without network I/O)
• Style: ⚠️ Indentation issues (run linter)
• Best Practices: ⚠️ Reducing test coverage without adding alternative coverage is generally discouraged

---

Verdict

Conditional Approval ✋ - The direction is right (removing flaky network tests), but:

1. ✅ Approve if: You're planning to add mock-based tests in a follow-up PR
2. ❌ Request changes if: This is the final state (coverage too low)

Please clarify your testing strategy and fix the formatting issues before merging.

---

🤖 Generated with Claude Code