This seems a little misleading as it's unknown, not zero. Feels like this should be left unset in the remote room cases that we don't fetch.

The default entry for _RoomEntry is ()

@attr.s(frozen=True, slots=True, auto_attribs=True)
class _RoomEntry:
    room_id: str
    # The room summary for this room.
    room: JsonDict
    # An iterable of the sorted, stripped children events for children of this room.
    #
    # This may not include all children.
    children_state_events: Sequence[JsonDict] = ()

which gets converted to json for the response by this:

    def as_json(self, for_client: bool = False) -> JsonDict:
        """
        Returns a JSON dictionary suitable for the room hierarchy endpoint.

        It returns the room summary including the stripped m.space.child events
        as a sub-key.

        Args:
            for_client: If true, any server-server only fields are stripped from
                the result.

        """
        result = dict(self.room)

        # Before returning to the client, remove the allowed_room_ids key, if it
        # exists.
        if for_client:
            result.pop("allowed_room_ids", False)

        result["children_state"] = self.children_state_events
        return result

So I think to leave children_state unset I think we'd have to change the structure of _Room_Entry to an optional and have as_json omit children_state in the case where self.children_state is None, but that seems clunkier than having the children_state be an empty list in the case of remote rooms we don't fetch - thoughts?

I am wondering if this could be addressed by adding a comment about the face that children_state is unknown in the case of remote rooms in the endpoint documentation?

After thinking about this more, I think we can just omit remote rooms altogether (no need for bare room_id entries). The client can still find the unknown remote rooms via the m.space.child in the children_state of the space. Further thoughts tracked in matrix-org/matrix-spec#2237

It still makes sense to call the option omit_remote_room_hierarchy and we just need to remove this part: #19021 (comment)

This is only testing the rooms from the first request 😬

rooms = rooms + new_rooms was removed in b187d6f

Perhaps we should just put this in a helper self._assert_expected_rooms_have_correct_fields(rooms) and call this after each request.

I changed it in the last round so that each request was being asserted on separately for only the rooms that should have been returned from each request - I think this should be fine and not necessitate a helper?

We do this for both tests so could simplify down to a helper.

Otherwise, we probably want to have an else clause for unknown rooms even though we're asserting specific rooms above in the assertCountEqual.

There is a an else clause to fail the return of an unknown room (line 356 and 271 - sorry can't figure out how to link a line in a PR diff in the UI), would that suffice?

(marking part to remove)

See other discussion, #19021 (comment)

After thinking about this more, I think we can just omit remote rooms altogether (no need for bare room_id entries). The client can still find the unknown remote rooms via the m.space.child in the children_state of the space. Further thoughts tracked in matrix-org/matrix-spec#2237

It still makes sense to call the option omit_remote_room_hierarchy and we just need to remove this part: #19021 (comment)