fix: Add Linux guards to network-dependent tests in HuggingFaceProvid…

[christopherkarani]

…erTests

On Linux, FoundationNetworking uses libcurl with try! internally, which causes fatal crashes (error code 43: CURLE_BAD_FUNCTION_ARGUMENT) instead of throwing catchable errors when network operations fail.

Tests that made real network calls assumed errors would be catchable: catch { // network errors expected }. This worked on macOS but crashed on Linux CI.

Changes:

• Add #if os(Linux) guards to 13 network-calling tests
• On Linux, tests now verify config/model validation without network calls
• On non-Linux platforms, tests continue to make network calls as before

Affected tests:

• testAcceptsHuggingFaceModelIdentifier
• testTextToImageAcceptsHuggingFaceModel
• testTextToImageWithDefaultConfig
• testTextToImageWithHighQualityConfig
• testTextToImageWithFastConfig
• testTextToImageWithSquare512Config
• testTextToImageWithSquare1024Config
• testTextToImageWithLandscapeConfig
• testTextToImageWithPortraitConfig
• testTextToImageWithCustomConfig
• testTextToImageWithEmptyPrompt
• testTextToImageProviderAvailabilityWithoutToken

🤖 Generated with Claude Code

[claude]

Pull Request Review: Linux Guards for Network-Dependent Tests

Summary

This PR adds #if os(Linux) guards to 13 network-dependent tests in HuggingFaceProviderTests.swift to prevent fatal crashes on Linux CI due to FoundationNetworking's internal try! usage with libcurl.

---

✅ Positive Aspects

1. Problem Understanding

• The PR correctly identifies a real Linux-specific issue with FoundationNetworking
• The explanation in the PR description is clear and accurate
• Comments in the code explain the rationale well

2. Consistent Pattern

• The approach aligns with existing patterns in LinuxCompatibilityTests.swift
• Guards are applied consistently across all affected tests
• Platform-specific behavior is well-documented

3. Test Coverage Preservation

• Linux tests validate core functionality (config/model validation) without network calls
• Non-Linux platforms continue to test network behavior as before
• No reduction in test coverage on supported platforms

---

🔍 Code Quality Observations

1. Test Value on Linux ⚠️

While the Linux test paths prevent crashes, some tests have minimal value. Examples:

Weak test (line 776-780):

#if os(Linux)
let config = ImageGenerationConfig.default
XCTAssertTrue(config.steps > 0, "Config should have default steps")
#else

This only validates that a constant has a positive value—not particularly useful.

Better test (line 867-871):

#if os(Linux)
let config = ImageGenerationConfig.square1024
XCTAssertEqual(config.width, 1024, "Square1024 should have 1024 width")
XCTAssertEqual(config.height, 1024, "Square1024 should have 1024 height")
#else

This validates actual configuration values—more meaningful.

Recommendation: Consider whether tests like testTextToImageWithDefaultConfig should be skipped entirely on Linux rather than having trivial assertions.

2. Comment Consistency ℹ️

• Lines 425-427, 736-737, 771-773 have similar comments
• Line 425 has a typo: / Note: should be // Note:
• Consider extracting to a single file-level comment

3. Test Naming 💭

Test names like testAcceptsHuggingFaceModelIdentifier don't reflect that they now do different things on different platforms. Consider:

• Documenting this in test comments
• Or creating separate Linux-specific tests

---

🐛 Potential Issues

1. Incomplete Guard in testTextToImageProviderAvailabilityWithoutToken ⚠️

Lines 1014-1027: This test has a guard but still calls provider.isAvailable on line 1012 before the guard:

let isAvailable = await provider.isAvailable  // Line 1012 - runs on ALL platforms
XCTAssertFalse(isAvailable, "Provider should not be available without token")

#if !os(Linux)  // Line 1014 - guard comes AFTER the network call
// Only attempt network call on non-Linux platforms

Problem: The isAvailable check might make a network request on Linux, potentially causing the crash this PR aims to prevent.

Recommendation: Move the isAvailable check inside the #if !os(Linux) block if it performs network I/O, or verify it's safe on Linux.

2. Missing Context on "Real Network Calls" 📝

The comments say these tests make "real network calls," but the non-Linux code uses token: "hf_test_token" which appears to be a fake token. The tests expect network/auth failures.

Question: Are these truly integration tests, or are they unit tests that happen to fail at the network layer? If they're meant to fail, consider using a mock/stub on all platforms.

---

🔒 Security & Performance

Security ✅

• No security concerns
• No exposure of credentials or tokens

Performance ✅

• Actually improves Linux CI performance by skipping network calls
• No performance impact on other platforms

---

🧪 Test Coverage Assessment

Coverage on Linux

• ✅ Model validation (identifier acceptance)
• ✅ Config object structure validation
• ❌ Network behavior
• ❌ Error handling for network failures
• ❌ API contract validation

Overall Impact

The reduced test coverage on Linux is acceptable given:

1. Linux is listed as "Partial" support in README (line 94)
2. The alternative is CI crashes
3. Core validation logic is still tested

However, consider future improvements:

• Mock HTTP layer to enable full testing on Linux
• Separate unit tests (validation) from integration tests (network)
• Use a testing HTTP server that doesn't trigger libcurl crashes

---

📋 Best Practices Assessment

Follows Repository Standards ✅

• Consistent with LinuxCompatibilityTests.swift patterns
• Aligns with documented Linux limitations (README.md lines 107-113)
• XCTest usage is standard

Code Style ✅

• Proper indentation
• Clear assertion messages
• Consistent guard usage

Documentation ⚠️

• Comments explain the "why" well
• Could benefit from a top-level comment explaining the pattern
• Minor typo on line 425

---

🎯 Recommendations

High Priority

1. Fix the isAvailable guard in testTextToImageProviderAvailabilityWithoutToken (lines 1012-1027)

Medium Priority

2. Review test value on Linux - Consider skipping tests with trivial assertions entirely
3. Fix comment typo on line 425
4. Consolidate duplicate comments - Extract common explanation to file header

Low Priority (Future)

5. Consider architectural improvements:
   • Mock HTTP layer for platform-independent testing
   • Separate unit tests from integration tests
   • Use protocol-based networking to enable test doubles

---

🎬 Verdict

Recommendation: Approve with minor fixes

This PR successfully addresses the immediate problem (Linux CI crashes) with a pragmatic solution that:

• ✅ Follows existing patterns
• ✅ Preserves test coverage on primary platforms
• ✅ Is well-documented
• ⚠️ Has one potential bug (isAvailable guard placement)

The approach is reasonable given the constraints, though there's room for future architectural improvements to enable better cross-platform testing.

Action Items Before Merge:

1. Verify/fix isAvailable call on line 1012
2. Fix comment typo on line 425

Great work on documenting the issue and implementing a consistent solution! 👍