Fix print issue on core manager repeated test

[xoriole]

Fixes #7510

Instead of patch decorator, context manager is used and mocked print object is used to check the call count instead of builtin print.

[drew2a]

The test, after being modified, appears to be logically similar to its previous version. Could you elaborate on why employing a context manager is deemed to be more effective compared to the decorator?

The correct version with the decorator could be:

@patch('builtins.print', new_callable=MagicMock, side_effect=OSError())
def test_on_core_read_ready_os_error_suppressed(mocked_print: MagicMock, core_manager):
    # OSError exceptions when writing to stdout and stderr are suppressed
    core_manager.app_manager.quitting_app = False
    core_manager.on_core_stdout_read_ready()
    core_manager.on_core_stderr_read_ready()
    assert mocked_print.call_count == 2

    # if app is quitting, core_manager does not write to stdout/stderr at all, and so the call counter does not grow
    core_manager.app_manager.quitting_app = True
    core_manager.on_core_stdout_read_ready()
    core_manager.on_core_stderr_read_ready()
    assert mocked_print.call_count == 2

[drew2a]

Related to #7495

[xoriole]

The real fix is to use the mocked print. The mocked object provided by a context manager patch or patch decorator should be logically the same.

assert mock_print.call_count == 2

The use of context manager is opinionated to make it a bit more explicit.

[drew2a]

This PR presents a resolution to the issue by utilizing mocked_print as opposed to the real one. Great discovery!

However, the choice between using a context manager and a decorator strikes me as more of a personal preference (I personally would vote for the decorator). As such, I propose we respect the original author's (@kozlovsky) choice of using the decorator.

As demonstrated below, it's possible to rectify the test while maintaining the original choice.

[kozlovsky]

@drew2a @xoriole
For me, the version with the patch context manager looks easier to understand because the mock object is explicitly created in the test.

The version with the new_callable argument of the patch decorator is also possible, but its difference with using the new argument is a bit harder to understand at first sight.

So, I prefer the context manager version now

[drew2a]

Would the next logical step then be to rewrite all tests, transitioning from the use of decorators to context managers?

[xoriole]

Would the next logical step then be to rewrite all tests, transitioning from the use of decorators to context managers?

IMO, it is just a single case and I would not think rewriting all tests is necessary.

[kozlovsky]

I mean, if we wrap a test with

@patch("some.function", MagicMock(...))
def test_function(mock):
    ...

We are creating a singleton mock object that we re-use if we re-run the same test multiple times.

The fact that this mock object is a singleton that is re-used multiple times is not entirely obvious.

When we use @patch("some.function", new_callable=MagicMock, side_effect=...), we avoid creating a singleton, as a new mock object is created each time the test function is called. But the purpose of using new_callable instead of new is not obvious to future readers, as a simple new argument works perfectly fine when the test is not re-run multiple times.

When the patch is used as a context manager, the mock object is created right inside the test, so it has no chance to be a singleton object by mistake when the test is re-run multiple times. Therefore, the patch-as-a-context-manager looks clearer to me than a patch-as-a-decorator when it is important for a mock object not to be a singleton by accident.

Would the next logical step then be to rewrite all tests, transitioning from the use of decorators to context managers?

If we really want to re-run each test multiple times, then yes, I think it is logical to replace all mocks that can break if they are singletons to mocks in patch-as-a-context-managers (not all mocks are vulnerable to the issue, so it is not necessary to replace all @patch to with patch, only a few ones)

But the bigger question is, should we really re-run the tests multiple times using this approach? We re-run tests only because we want to determine flaky tests. But flaky tests when the entire test suite is started for a single time are flaky for a different reason than when we use pytest-repeat.

I think it may be better for us to find another way of re-running the same test multiple times when the global variables and singleton mocks do not affect the test execution. If we want to find flaky tests, it is better to re-run the entire test suite multiple times and not the individual tests inside a single test suite run.

[drew2a]

@kozlovsky

The issue did not lie in how the mock was defined, but in the way the print calls count was verified.

As @xoriole pointed out,

The real fix is to use the mocked print.

The decorator functions properly with multiple calls, as confirmed by the fact that all other tests operate correctly.

The construct used in the following example:

@patch('builtins.print', new_callable=MagicMock, side_effect=OSError())
def test_on_core_read_ready_os_error_suppressed(mocked_print: MagicMock, core_manager):
    ...

was chosen because it allows the use of a mocked object (in this case, mocked_print) within a function.

The example you've given is not accurate. A mocked object cannot be used in the following way:

@patch("some.function", MagicMock(...))
def test_function(mock):
    ...

If this kind of definition is employed, it should be written as:

@patch("some.function", MagicMock(...))
def test_function():
    ...

[drew2a]

@xoriole

IMO, it is just a single case and I would not think rewriting all tests is necessary.

By transitioning from a decorator to a context manager, the consistency of the tests is diminished. Currently, most tests in this file use decorators, with only one employing a context manager.

This could lead to confusion for future readers of this file, prompting them to question why one test differs from the others. Is there a valid reason for this disparity? By maintaining a consistent style for our tests, we can preemptively address these types of questions.

[kozlovsky]

@drew2a First, sorry for the mistake; I misremember the API of @patch when the new argument is used. But it does not affect my main point.

The issue did not lie in how the mock was defined, but in the way the print calls count was verified.
As @xoriole pointed out,

The real fix is to use the mocked print.

Here, I have an impression our understanding of the reason for the problem is different. My point is that the mocked print was used all the time, even in the original version of the test.

In the original version of the test:

@patch('builtins.print', MagicMock(side_effect=OSError()))
def test_on_core_read_ready_os_error_suppressed(core_manager):
    ...
    assert print.call_count == 2

In the expression assert print.call_count == 2, print is a mock object. The fact that it is not passed as an argument of the test function is not relevant at all. What is relevant is when this mock object is created.

When @patch is used as a decorator, we have several slightly different ways of how it is applied.

1. With a target argument only:

@patch('tribler.core.sentry_reporter.sentry_reporter.sentry_sdk.init')
def test_init(mocked_init: Mock, sentry_reporter: SentryReporter):
    ...

2. With a second new argument (sometimes written as a second positional argument):

@patch('binascii.unhexlify', new=Mock(side_effect=binascii.Error))
def test_parse_magnetlink_binascii_error_40(caplog):
    ...

3. With a new_callable keyword argument:

@patch.object(AsyncGroup, 'cancel', new_callable=AsyncMock)
async def test_multiple_shutdown_calls(async_group_cancel: AsyncMock):
    ...
    async_group_cancel.assert_called_once()

In all three forms, the mock object is created and used instead of the original object/function. In the second form, when the new argument is used, the mock is not passed to the function in a corresponding argument. But why? Does it mean that the mock object is not created? No, it means that the fully constructed singleton mock object was provided by us when we specified the @patch decorator. For that reason, the mock library believes we already have a mock object and it is not necessary to pass it as an argument. That is, the second test:

@patch('binascii.unhexlify', new=Mock(side_effect=binascii.Error))
def test_parse_magnetlink_binascii_error_40(caplog):
    ...

can be rewritten as

SINGLETON_MOCK=Mock(side_effect=binascii.Error)
@patch('binascii.unhexlify', new=SINGLETON_MOCK)
def test_parse_magnetlink_binascii_error_40(caplog):
    ...
    assert binascii.unhexlify is SINGLETON_MOCK
    SINGLETON_MOCK.assert_called()

Inside the test, there is a mock object, and we can access it using binascii.unxelify or using a global variable SINGLETON_MOCK. But the problem is it is a singleton that is reused on each repeated call of the test function. Originally, before we started calling the same test function multiple times in a single test run, it was not important to us that it was a singleton.

Consider a similar test with the same problem:

@patch_import(['faulthandler'], strict=True, enable=MagicMock())
@patch('tribler.core.check_os.open', new=MagicMock())
def test_enable_fault_handler():
    import faulthandler
    enable_fault_handler(log_dir=MagicMock())
    faulthandler.enable.assert_called_once()

Here, we test faulthandler.enable.assert_called_once() in the last line. faulthandler.enable is a singleton mock that is defined in the first line with the @patch decorator. So, the test function is equivalent to the following:

SINGLETON_MOCK=MagicMock()
@patch_import(['faulthandler'], strict=True, enable=SINGLETON_MOCK)
@patch('tribler.core.check_os.open', new=MagicMock())
def test_enable_fault_handler():
    import faulthandler
    enable_fault_handler(log_dir=MagicMock())
    SINGLETON_MOCK.assert_called_once()

And this function has the same problem with repeated test calls; it works correctly on the first call only. But it uses the mock, so the problem is not that the mock is not used; the problem is the mock is not re-created each time the function is called.

So we need to have a fresh mock in order to have the correct result for mock.assert_called_once(). It can be achieved in three ways:

1. Choose a form of @patch decorator that returns a new mock instance as an argument.
2. Use the @patch decorator as a context manager and create an instance of mock right inside the test.
3. Reset the mock object at the beginning of the test.

In this specific case, as the @patch_import decorator is used instead of the @patch decorator, the third way looks the easiest:

@patch_import(['faulthandler'], strict=True, enable=MagicMock())
@patch('tribler.core.check_os.open', new=MagicMock())
def test_enable_fault_handler():
    import faulthandler
    faulthandler.enable.reset_mock()  # allows to re-run the same test multiple times
    enable_fault_handler(log_dir=MagicMock())
    faulthandler.enable.assert_called_once()

And I actually like the reset_mock() call for its explicitness. But two other ways are also possible in other tests.

I actually do not have objections to the new_callable argument of the @patch decorator; we can use it. And it is not necessary for us to replace all @patch decorators with context managers. The problem is relevant only to a subset of tests when we use specific mock methods like assert_called_once that can be affected by a re-using of the same mock object in multiple test executions.

So, if we believe that new_callable is better than a context manager, I'm fine with this. I have an impression that the reason why we use new_callable instead of new may be slightly non-obvious for an unaware reader who does not know that we sometimes re-run the same test multiple times. But I think it is not a big deal.