Should TestStore.state be public?

[mbrandonw]

I'm starting this discussion based on #1122, which publicizes TestStore.state so that assertions can be made outside of .send and .receive assertion blocks. We don't want to merge such a PR without understanding its use cases. From our perspective there are never any state mutations between .send and .receive blocks, and so there should never be a need to make an assertion outside those blocks.

So, those who have done this, can you share some examples of when and how you have done this?

[jshier]

Copying over my response.

Two points.

First, personally, I dislike any test structure that puts assertions out of line with the main test execution. I consider TCA's mutation checkers to themselves be assertions, so adding additional assertions conflates requirements, sort of like nesting assertions directly. To me this:

testStore.receive(...) {
  $0.value = newValue
}
XCTAssertEqual(testStore.state.otherValue, someOtherValue)

is cleaner and clearer than

testStore.receive(...) {
  $0.value = newValue
  XCTAssertEqual($0.otherValue, someOtherValue)
}

as it properly separates my assertions about mutations and other assertions. Technically, yes, they do the same thing, but clarity is paramount in tests.

Second, it fundamentally seems wrong to assert against the locally mutated value instead of the actual underlying state. It conflates the action I took as the one writing the test and what actually happened under the hood. Technically, if the mutation test passed, then the local value should be correct as far as the rest of the assertions go, but actually connecting back to the "real" state seems more correct to me.

[stephencelis]

I have a different perspective on the first point, since the main purpose of the blocks is to assert against state when it updates.

The second point I find pretty compelling though! We'll mess around with stuff to make sure we're not introducing anything that could cause problems.

[mbrandonw]

@jshier In such a situation, state.otherValue could have only changed from a previous action (or is still the default value from when the TestStore was created), and so that field has already been asserted on. What is the use case for asserting on it again?

The only thing that comes to mind is if it's a reference type, but I think that is going to be problematic for a lot more reasons than where an assertion statement is placed.

It's worth noting that a strong reason to not publicize is that it gives people an escape hatch to make their tests pass without asserting on anything:

store.receive(...) {
  $0 = store.state
}

That will always pass.

[jshier]

Computed properties, typically. I usually put a formattedX property on my leaf models, so it's useful to assert that the formatted version has properly changed when the stored value does.

But like I said, my support is mainly around "proper" structure rather than any technical capability.

As for your example, users can already do that by capturing a state value from the previous assertions, so if you want to avoid it completely you still need a solution. Perhaps a wrapper that prevents setting the whole value at once or detects when the user does so?

[mbrandonw]

Computed properties, typically. I usually put a formattedX property on my leaf models, so it's useful to assert that the formatted version has properly changed when the stored value does.

Ah good point. Though I suppose one could just write separate unit tests only on those computed properties in isolation.

As for your example, users can already do that by capturing a state value from the previous assertions, so if you want to avoid it completely you still need a solution. Perhaps a wrapper that prevents setting the whole value at once?

Haha yeah, you can escape the state from a previous assertion and then manually run the reducer on it, but that's a lot more clearly a weird thing to do than $0 = store.state.

[mbrandonw]

We just investigated this a bit, and things are a little weirder than I first thought. Turns out that store.state is the previous state in store.send {...} but the next state in store.receive {...}. Which means my fear of an escape hatch was only applicable to store.receive.

Having seen that we realized there are small changes we can make so that store.send {...} and store.receive {...} do not get access to the new value, which closes the escape hatch and means we are a lot more open to this change now.

We pushed a branch here that we could clean up and PR. Curious if anyone else has any other thoughts.

[jeffersonsetiawan]

I've encounter the same problem like this in my project, and usually I just did the same.

testStore.receive(...) {
  $0.value = newValue
  XCTAssertEqual($0.otherValue, someOtherValue)
}

and several weeks ago I come up with the idea of eliminating the computed property using ViewState.

For example:

struct Item {
	var id: String
	var name: String
	var qty: Int
	var price: Int
}

struct State {
	var items: [Item]

	var totalPrice: Int {
		items.reduce(0, { acc, item in
			acc + item.qty * item.price
		})
	}
}

I move the totalPrice into the ViewState, as the totalPrice is actually derived from the items property.

struct Item {
	var id: String
	var name: String
	var qty: Int
	var price: Int
}

struct State {
	var items: [Item]

	var totalPrice: Int {
		items.reduce(0, { acc, item in
			acc + item.qty * item.price
		})
	}
}

struct ViewState {
	var items: [Item]
	var totalPrice: Int

	init(state: State) {
		self.items = state.items
		self.totalPrice = state.items.reduce(0, { acc, item in
			acc + item.qty * item.price
		})
	}
}

and in the unit test, I've ended up like this:

func testSomething() {
	let testStore = TestStore().scope(ViewState.init)

	testStore.send(.foo) {
		$0.items = [expectedItems]
		$0.totalPrice = 100_000
	}
}

The disadvantage for this is usually you need to have 2 test, 1 for core, and other one is for the ViewState (like in the isowords GameCore/GameView).

What do you think about this style?

[alexito4]

I agree that this is a fine alternative for some cases, but IMO I tend to use the ViewState when I need to reduce the scope of what the view sees. If the view needs to use everything in the state + a computed property this ends up being too much boilerplate and maintenance burden. So I like it as an alternative for some occasion, but I would still love for the TCA to let me decide when to use one or the other without friction on the tests.

[jeffersonsetiawan]

I'm agree with you.
In my company, this is not a mandatory because of the boilerplate too.

[alexito4]

Hello, thanks for opening this discussions folks! This is something I've wanted for a while, and in fact is the thing I was interested on this alternative version of the TestStore.

I agree with the points raised that this is not something that should be overused, but is not so rare to find myself needed it. Granted, maybe I'm doing something wrong, maybe there is an alternative for all those occasions... but even so, If I've done something "weird" I want to rely on my tests to make sure the weirdness works, having a test framework that doesn't let me do that is a bit of a pita.

Since you asked for examples the one that I have right now in mind from yesterday (which I think is similar to my previous needs):

I want to check the state of the Store just after initialization even before any action is sent. I can't check this inside a send/receive because I don't want to interact with the store just yet. This helps me a lot making sure the preconditions of my domain are satisfied. I find that inits a great place to add some small logic for specific preconditions (limit the amount of items in a list, precompute some values, etc), as long as the environment is not required.

The alternatives of being forced to go trough the reducer instead just for this we then have to make "view appeared" events that just add noise most of the time, and then we have a period (init -> view appeared) where the state is in an inconsistent state. The other alternative of using a ViewState is also too much boilerplate for simple cases where you just want to precompute one thing and the view still needs access to the rest.

All in all it feels like the restriction of the TestStore adds too much friction to some designs, I would understand if that's intended (APIs should nudge us in the right direction) so maybe it's a me problem. I just found myself needing this too often so I thought to shout about it :P

Thanks 🤗

[tgrapperon]

I want to check the state of the Store just after initialization even before any action is sent.

Isn't the store's state always exactly the same at this point as the one you provided as initialValue when creating the TestStore?

[stephencelis]

@tgrapperon While that's true, I can see stylistic/ergonomic reasons for wanting to initialize state alongside the store. After working out some concerns we think it's probably harmless to support this feature.

[tgrapperon]

@stephencelis, I agree with the ergonomics. This was a direct question to make sure that I didn't missed something about how TCA works.

[tiagomnh]

My scenario is also related to computed properties. For example, I have a state that olds the value of multiple feature flags and a computed property that sets up a list based on the flag values.

struct ListState {
    var featureAIsAvailable: Bool
    var featureBIsAvailable: Bool
}

extension ListState {
    var rows: [Row] {
        [
            featureAIsAvailable ? Row("Option A") : nil,
            featureBIsAvailable ? Row("Option B") : nil,
            Row("Option C"),
            Row("Option D")
        ]
        .compactMap { $0 }
    }
}

func testOpenListAndFlagValueChanges() {
    let featureFlags = FeatureFlags()

    let testStore = TestStore(initialState: ListState(), reducer: listReducer, environment: .tests(flags: featureFlags))

    // Doesn't actually change the state; used by a higher-order reducer that handles analytics.
    testStore.send(.onAppear)

    XCTAssertEqual(testStore.state.rows, ["Option C", "Option D"])

    featureFlags.featureAIsOn = true

    testStore.receive(.featureAFlagChangedValue(true)) {
        featureFlags.featureAIsAvailable = true
    }

    XCTAssertEqual(testStore.state.rows, ["Option A", "Option C", "Option D"])
}

But just like was mentioned by @jshier and @mbrandonw, I could definitely test the computed property in its own unit test without the TestStore at all. I could have also moved the initial state to a local variable and assert the initial state without accessing the testStore.state. Follow-up assertions after state changes could be done inside the send/receive closures as well.

I think having it like I'm presenting in my example is more clear to me and helps me reason about the test, but it is definitely not a huge thing and there are many ways of working around it like I've mentioned.

Thanks for considering this, though! 🙏 Awesome work, even if we end up not merging it in.

[mbrandonw]

We just opened a PR to fix the escape hatch and make state public: #1127