Improve HaversackEphemeralStrategy ergonomics

[JacobHearst]

Having recently written some unit tests using the ephemeral strategy, I found myself wishing that I didn't have to commit a bunch of difficult to read strings for repeatable access to the mock data. To address this, I've introduced a subscript to HaversackEphemeralStrategy so that developers can access mockData via the queries they already have.

I did consider adding a pair of setValue/getValue functions but thought that the subscript access would be more ergonomic, especially considering that HaversackEphemeralStrategy is essentially just a wrapper around mockData (excluding key import/export).

[macblazer]

The added unit tests show that the subscript code can get the values that are set with the subscript code, but they do not show that they coordinate exactly with the long key names. Can you please add one more test such as testGenericPWSearchReferenceMock() where you set the mock data with the subscript, then call a Haversack function that will use the ephemeral strategy to return the mocked value?

Basically a copy of testGenericPWSearchReferenceMock() but with line 36 changed to use the nicer query-based subscript instead of a long strange string.

[JacobHearst]

The added unit tests show that the subscript code can get the values that are set with the subscript code, but they do not show that they coordinate exactly with the long key names. Can you please add one more test such as testGenericPWSearchReferenceMock() where you set the mock data with the subscript, then call a Haversack function that will use the ephemeral strategy to return the mocked value?

Basically a copy of testGenericPWSearchReferenceMock() but with line 36 changed to use the nicer query-based subscript instead of a long strange string.

Perhaps as you suspected @macblazer, writing this unit test revealed that the current implementation is functionally useless because Haversack adds additional keys to the query before sending the query to its strategy. I still think HaversackEphemeralStrategy could benefit from some ergonomic improvements so here are some alternatives that I would like feedback on:

1. Add a layer of indirection in Haversack so that there's a way to generate the final query without actually calling through to the configured strategy
2. Add a method that:
   • Accepts a mock value and a throwing closure
   • Catches a "missing value for key" error from the closure and sets the value for the key in the error to the mock value

Some mostly real sample code to better visualize these options:

// In the HaversackMock library
extension Haversack {
    // Alternative 1
    public func mockFirst<T: KeychainQuerying>(where query: T, value: Any) throws {
        let query = try makeSearchQuery(query, singleItem: true)
        let ephemeralStrategy = configuration.strategy as! HaversackEphemeralStrategy
        let key = ephemeralStrategy.key(for: query)
        ephemeralStrategy.mockData[key] = value
    }

    // Alternative 2
    public func mock(value: Any, forErrorThrownFrom operation: () throws -> Void) rethrows {
        let ephemeralStrategy = configuration.strategy as! HaversackEphemeralStrategy

        do {
            try operation()
        } catch let error as NSError {
            guard <error is "missing value for key" error> else {
                throw error
            }

            ephemeralStrategy.mockData[<missing key>] = value
        }
    }
}

// In unit tests
// Alternative 1
func testPasswordQuery() throws {
    let query = GenericPasswordQuery()
    let mock = HaversackEphemeralStrategy()
    let haversack = Haversack(configuration: .init(strategy: mock))

    let expectedEntity = GenericPasswordEntity()
    try haversack.mockFirst(where: query, value: expectedEntity)
}

// Alternative 2
func testPasswordQuery() throws {
    let query = GenericPasswordQuery()
    let mock = HaversackEphemeralStrategy()
    let haversack = Haversack(configuration: .init(strategy: mock))

    let expectedEntity = GenericPasswordEntity()
    try haversack.mock(value: expectedEntity) {
        _ = try haversack.first(where: query)
        _ = try haversack.first(where: KeyQuery()) // Invalid usage, this line will never run
    }

    let password = try haversack.first(where: pwQuery)
    XCTAssertNotNil(password)
}

I think I prefer alternative 1 because it has less room for developer error. As I called out in the test example for alternative 2, you can perform as many operations on your Haversack as you like but only the first throwing operation's error will be caught and have its value mocked.

[macblazer]

Alternative 1 is definitely better.

The subscripting behavior can be removed, and some methods added to more easily set mock data into the strategy would be better. The second parameter might be better as mockValue: instead of simply value. This would read more explicitly as try haversack.mockFirst(where: query, mockValue: testingValue).

Instead of the force typecast to the HaversackEphemeralStrategy I recommend using an as?. Then if it fails you can trigger an explicit testing failure with more info because that is a developer error if they are using a different strategy and trying to mock during the unit tests. The force cast will simply crash the unit test with no usable information for the developer.

[JacobHearst]

Instead of the force typecast to the HaversackEphemeralStrategy I recommend using an as?. Then if it fails you can trigger an explicit testing failure with more info because that is a developer error if they are using a different strategy and trying to mock during the unit tests.

100%, the force cast was for simplicity while brainstorming