consider adding facilities to inspect all statuses added to the collect status event.

[PietroPasotti]

should we add something to facilitate verifying all of the individual statuses that went into the collect-status event, or are we happy with verifying that the highest priority one is what we think it is?
Food for thought.

Originally posted by @PietroPasotti in #78 (comment)

[benhoyt]

I think we should only verify the resulting status, otherwise it seems like we're testing implementation details. It's not a log where you may want to verify logs A, B, and C were sent out. A charm could call add_status(ActiveStatus()) 1 time or 5 times, and it should have the same result.

[dimaqq]

Countering the above, we've had a discussion where a following hypothetical was presented:

• suppose a charm has a bunch of containers, services, dependencies, etc.
• the API provides a facility to assert on the aggregate of the N status values above
• the charmer ends up writing very peculiar test setup to catch the specific status bit

Here's an example off the top of my head:

        # Common setup
        kafka_broker = MockKafkaBroker.return_value
        zookeeper_client = MockZookeeperClient.return_value

        zookeeper_client.is_connected.return_value = True
        kafka_broker.is_healthy.return_value = True
        kafka_broker.get_replication_status.return_value = 'healthy'

        # Test 1
        kafka_broker.is_healthy.return_value = False
        kafka_broker.health_check.side_effect = TimeoutError("Broker health check timed out")
        assert XXX

        # Test 2
        kafka_broker.get_disk_usage.return_value = 95  # Percentage used
        assert YYY

        # Test 3
        kafka_broker.get_replication_status.return_value = 'stuck'
        kafka_broker.get_replication_error.return_value = "Replication slot is stuck"
        assert ZZZ

A multi-error API would allow tests like:

    # Test 1
    assert "kafka is not healthy" in foo.status_elements
    assert "timed out" in foo.status_elements

While a single-error API requires very specific test setup, e.g.

    # Test 1
    # Lie about healthy status to have the timeout bubble up
    kafka_broker.is_healthy.return_value = True
    kafka_broker.health_check.side_effect = TimeoutError("Broker health check timed out")
    
    assert "timed out" in foo.combined_status

and separately

    # Test 1
    kafka_broker.is_healthy.return_value = False
    kafka_broker.health_check.return_value = True
    
    assert "not healthy" in foo.combined_status

I could be a small issue or a bigger one, depending on the tooling to mock out specific workload client API.

Hand-written mocks probably don't care as much.

On the other hand, if charmer used some off-the-shelf mocks, like the responses library, ensuring a very specific mock behaviour is a bit harder.

WDYT?

[benhoyt]

Yeah, I can see what you're saying -- with only a single status, the person writing tests might have to know more about the implementation details to write the test than with a list of statuses. Would be interested to hear what @tonyandrewmeyer has to say, and @PietroPasotti -- in practice, which approach do you think would require you to have to know more implementation details about the charm (and therefore be a brittler test)?

[PietroPasotti]

I think a feature of this kind would allow you to write simpler tests.
Suppose your charm has three failure modes: a relation is missing (blocked), an API is down (waiting), a container cannot connect (waiting). I want to test that if the API is down, then the charm sets waiting "API is down". However the final status might be blocked unless we also take care to mock container connectivity.
If we could do something like

assert WaitingStatus("API is down") in context.collected_statuses

then we wouldn't have to also mock relation and container when we're testing the status that depends on the API.
Also, suppose tomorrow we add one more blocked failure mode, now we need to update all status tests to also mock that piece of state without which the charm will set a different blocked.

Alternatively we could recommend folks to redesign their code and have the charm define separate collect_api_status, collect_relation_status, collect_container_status methods that we can patch and assert_called_with(...) instead.
This approach however feels even more brittle, more unit, less scenario, less black-box.

[benhoyt]

Thanks. I'm convinced by Dima and Pietro's arguments here -- this would make testing that kind of thing less fragile.

[tonyandrewmeyer]

A charm could call add_status(ActiveStatus()) 1 time or 5 times, and it should have the same result.

For what it's worth, I think this could be a set (all the examples above are x in collected_statuses, and I think that is the expected use-case - checking that the statuses were added in a specific order seems a bad pattern), so if the charm added the same status (name & message) multiple times that would have the same result as doing it once.

[benhoyt]

Ah yes, very much agreed on set (unordered) vs list (ordered).

[tonyandrewmeyer]

Moved to canonical/operator#1426