Increase test coverage

[weklund]

This is a working PR to incrementally improve the test coverage to a percentage that we want. I imagine once we reach this predefined goal, then adding that as a minimum level should fail future CI actions.

[weklund]

As of last commit we are at 75%

[weklund]

Now at 78%

I believe I've capture a lot of the easy wins for unit testing. I want to tackle logs.py errors.py and the mixins next.

Happy to receive feedback so far!

[Voyz]

Hey @weklund great initiative 👏

1. Did you write these tests by hand or are they AI-generated? If you used AI, did you review it one by one?
2. I thought we want to move away from unittest and towards pytest. We've established that both can live safely, so moving towards pytest for any new tests would make sense, right? Happy to discuss this if I'm misunderstanding something here
3. I'm trying to adopt the Arrange, Act, Assert sections you've suggested. Possibly we could start using them with these new test here?

[weklund]

1. Some of these are AI generated, and should be reviewed more :)
2. No you're right... I already forgot ha. Let me fix that.
3. No you're right!

Let's try this again :)

[weklund]

Updated subscription controller, will update the oauth tests next

[weklund]

@Voyz I did another pass here, and a slightly different approach. Used AI a bit, but instead of writing, I used it to refactor, reduce, and simplify what the tests were asserting. Curious on your thoughts here. Unfortunate the diff is pretty large, but I have reached 84% coverage now.

Looks like the next biggest gains would be from ws testing, but wanted to follow up on client work in a different PR.

In addition I wanted to wait on adding through coverage to mixins.

Would love your thoughts here! :D

[weklund]

Some actions to take for this PR:

• Review tests and ensure we mock as little as possible
• See more test candidates for parameterize
• Add no coverage where it makes sense

[weklund]

@Voyz Would love your thoughts :)

[Voyz]

hey @weklund thanks for your patience! I'm done with the move, so got some time to look into your PR again. I've done the first round of review, but admittedly didn't make much progress and didn't get a chance to finish it yet. It is a little difficult to follow through the tests due to the way they are structured with nested fixtures and large parameterise lists. I've brought it up in several points, giving examples on how to structure them in a way that would make the tests more standalone and readable.

Additionally, there seem to be the cases of 'testing the mocks, not testing the functionality' we've discussed last time, as well as tests that mock what has already been mocked. You previously mentioned these were due to the fact that the original PR was written by your AI agent without you having reviewed it. Was this also the case here?

[Voyz]

If we parameterise them, then let's add these to var.py

[Voyz]

Although quick and helpful, this is a bit hacky. Overloading function definition with quasi-class staticmethod-like lambdas is not idiomatic and is going to be harder to maintain than doing it explicitly as functions/classes.

A quick alternative:

class SubscriptionFactory:
    """Factory for creating subscription data structures with common patterns."""

    @staticmethod
    def make(
        status: bool = False,
        data: Optional[Dict[str, Any]] = None,
        needs_confirmation: bool = True,
        subscription_processor: Optional[Callable] = None,
        channel_suffix: str = ""
    ) -> Dict[str, Any]:
        """Create a subscription dictionary with standard structure."""
        return {
            "status": status,
            "data": data or {"key": f"value{channel_suffix}"},
            "needs_confirmation": needs_confirmation,
            "subscription_processor": subscription_processor,
        }

    @classmethod
    def active(cls, processor: Optional[Callable] = None, data: Optional[Dict[str, Any]] = None) -> Dict[str, Any]:
        return cls.make(status=True, data=data, needs_confirmation=True, subscription_processor=processor)

    @classmethod
    def inactive(cls, processor: Optional[Callable] = None, data: Optional[Dict[str, Any]] = None) -> Dict[str, Any]:
        return cls.make(status=False, data=data, needs_confirmation=True, subscription_processor=processor)

    @classmethod
    def active_no_confirm(cls, processor: Optional[Callable] = None, data: Optional[Dict[str, Any]] = None) -> Dict[str, Any]:
        return cls.make(status=True, data=data, needs_confirmation=False, subscription_processor=processor)

    @classmethod
    def inactive_no_confirm(cls, processor: Optional[Callable] = None, data: Optional[Dict[str, Any]] = None) -> Dict[str, Any]:
        return cls.make(status=False, data=data, needs_confirmation=False, subscription_processor=processor)


@pytest.fixture
def subscription_factory() -> SubscriptionFactory:
    """Pytest fixture exposing the subscription factory."""
    return SubscriptionFactory

[Voyz]

this fixture doesn't seem to be used anywhere?

[Voyz]

I'm thinking a bit about the readability and maintainability of these tests. It heavily uses the pytest fixtures, which is great, but it does turn it into a little bit of a puzzle to deduce what values are set to.

Take for example second case:

 # Data only
    ({'data': {'modified': 'data'}}, False, {'modified': 'data'}, True, False),

To start with, for this chain of values it is not immediately apparent which variables they map to - I need to spend a while looking back and forth to deduce these which test function parameters they correspond to. I wonder whether dropping @pytest.mark.parametrize for such high number of arguents wouldn't be more readable:

def test_modify_subscription_parameters_status_only(controller_with_test_subscription):
    controller_with_test_subscription.modify_subscription('test_channel', {'data': {'modified': 'data'}})
    ... 
    assert subscription['status'] is True
    ... #etc.

Irrespectively of above, because of nested fixtures figuring out what the expected values should be is also a bit of mental gymnastic, I literally just had to go through this process when reviewing this code:

• What is the expected status here in the second case?
• expected_status... Which variable is that? Ah.. it's False
• Hold up, why is status expected as False?
• Let me see controller_with_test_subscription
• Ah it is intialised with subscription_factory.inactive()
• Let me see subscription_factory
• [I'm skipping the figuring out the function overloading with lambdas here because I wrote about it above, but it's also a step]
• Ah inactive is a lambda function, calling create_subscription
• Let me see create_subscription
• Ah status is False by default

And similar back and forth needs to happen for many other parameters, even once we figure out they originate from subscription_factory and create_subscription.

Although I'm all for DRY and wrapping repeated code into helper functions, here I think it may be going a step too far. I therefore think, these tests would be cleaner if we made things a little more explicit:

@pytest.fixture
def controller(mock_processor, subscription_factory):
    """Create a SubscriptionController with a predefined test subscription using factory."""
    controller = SubscriptionController(subscription_processor=mock_processor)
    # Add send method since SubscriptionController is a mixin expecting WsClient
    controller.send = MagicMock(return_value=True)
    controller.running = True  # Default to running state
    return controller


@pytest.fixture
def default_subscription():
    return {'status': False, 'data': None, 'needs_confirmation': True, 'subscription_processor': None}


def test_modify_subscription_status(controller, default_subscription):
    # Arrange
    controller._subscriptions['test_channel'] = default_subscription.copy()
    modification = {'status':True}

    # Act
    controller.modify_subscription('test_channel', modification)

    # Assert
    assert controller._subscriptions['test_channel'] == (default_subscription | modification) # or {**default_subscription, **modification}
    
    
def test_modify_subscription_data(controller, default_subscription):
    # Arrange
    controller._subscriptions['test_channel'] = default_subscription.copy()
    modification = {'data':{'modified': 'data'}}

    # Act
    controller.modify_subscription('test_channel', modification)

    # Assert
    assert controller._subscriptions['test_channel'] == (default_subscription | modification)
    
    
def test_modify_subscription_needs_confirmation(controller, default_subscription):
    # Arrange
    controller._subscriptions['test_channel'] = default_subscription.copy()
    modification = {'needs_confirmation':False}

    # Act
    controller.modify_subscription('test_channel', modification)

    # Assert
    assert controller._subscriptions['test_channel'] == (default_subscription | modification)
    
    
def test_modify_subscription_multiple_parameters(controller, default_subscription):
    # Arrange
    controller._subscriptions['test_channel'] = default_subscription.copy()
    modification = {'status': True, 'data': {'new': 'data'}, 'needs_confirmation': False}

    # Act
    controller.modify_subscription('test_channel', modification)

    # Assert
    assert controller._subscriptions['test_channel'] == (default_subscription | modification)

While this introduces a little more repetition, each test becomes much more self-contained and easy to follow - which in general should be a principal philosophy behind tests if I'm not mistaken.

Yes, there still is a bit of DRY and following through to fixtures, but I feel that reading defaults from default_subscription which is directly referenced in each test is cleaner than having to go through 3 fixture redirections before finding defaults and expected values.

I think @pytest.mark.parametrize is great when there are a few parameters but a lot of combinations (like a string-parsing function which takes one parameter, and could accept 10 different forms) - I use it frequently too. But here this kind of long argument list along with the heavy fixture nesting could make debugging errors harder than it needs to be if we made things more explicit accepting a little bit of repetition.

[Voyz]

Why would you write it out between three lines like this? It seems a bit verbose to me, but maybe you had some reasoning behind writing it like so? I'd think of a simple:

assert f'Subscription {nonexistent_channel } does not exist. Current subscriptions:' in error_message 

[Voyz]

Why would you put a section header with only one test if other such cases don't carry a header? Were there more tests here before?

[Voyz]

This also bears the @pytest.mark.parametrize readability difficulty I've mentioned earlier.

Then:

• Why the 'Mock only external dependencies' by mocking send which already is mocked?
• And why not mock the _send_payload if in other unit tests we mock methods like this? That method just forward return status from send anyway in essence, we're not really testing anything here by keeping it unmocked, are we?

Finally, this test feels a bit like testing whether we mocked things correctly, more than testing the functionality in question. If we mock the wait_until to return mocked values, and do nothing about the self.running, then _attempt_unsubscribing_repeated essentially becomes a for loop where we indirectly call a send mock - and this test just tests whether we can call a mock in a loop, and how many times we called that mock. This is redundant.

It's the wait_until, is_subscription_active and is_running that make this worth testing. If we mock or ignore all these, what is the reasoning behind writing such test?

[Voyz]

send is already a mock, we could just subscription_controller.send.return_value = subscribe_success

[Voyz]

similarly, _subscription_processor is already a mock, from mock_processor. We can just subscription_controller._subscription_processor.make_subscribe_payload .return_value = 'test_payload'

[Voyz]

Forgive me if I'm missing something here - once again the pytest nesting and trying to handle all @pytest.mark.parametrize cases in the same test makes things a little harder to reason about - but I don't think this is sufficient to test this properly. Is this test actually passing?

The _attempt_subscribing_repeated is expected to wait until a confirmation which should only happen once a message is received through and parsed by IbkrWsClient._on_message(). Just returning success indicates that the status was modified, but since _on_message was never called the status should still be False.

If it passes, it's most likely only because recreate_subscriptions filters subscriptions for ones that have status=False. Then, you check if after calling recreate_subscriptions the status is equal to that of initial_subscriptions, which will always be False due to aforementioned filter. No actual subscription recreation is tested, it tests that subscriptions that weren't active stay inactive after theoretically having them 'recreated'.

Can you explain what was your logic in covering it in this way? Maybe I'm not seeing something here