Adding unit tests for the ReportSubscriber

[JonasLudwig1998]

No description provided.

[biozshock]

This and next similar tests looks like a "coverage only" tests. There is no clear connection between methods mocked an the outcome.
For example here: why the in is called two times, but you return same 'a'? How the expression builder mock corresponds with the incoming data? What is the expected result? What are the expectations of this test case?

[JonasLudwig1998]

Yeah, this one servers as a coverage only test

[JonasLudwig1998]

The returns 'a' is just used because the test would't continue if no return value was specified for the method

[biozshock]

Well, to make it somewhat reliable, you should not only check for the methods called, but also the result of the call. Now when somebody refactors the method under test they can introduce new bugs no problem.

[JonasLudwig1998]

Here we really wanted to test if the logic in the function is working but not if the doctrine components are working properly. For this, we'd need integration tests, not unit tests. I see the point, and of course it would make sense to implement it. I'll ask the corresponding pm if it makes sense to allocate the resources to implement an extra integration test

[biozshock]

There is no need to use integration tests. We already have all information that we need, so this is just a puzzle.

public function testGetCompanySegmentConditionWhenOperatorEqualsIn(): void
{
    $filterSubQueryResult = 'filter sub query';
    $filterResult = 'filter result';
    $subQueryResult = 'sub query';
    $filterValue = '3';

    $this->queryBuilderMock->expects(self::once())
        ->method('andWhere')
        ->with($filterSubQueryResult);
    $this->queryBuilderMock->expects(self::once())
        ->method('getSQL')
        ->willReturn($subQueryResult);

    // What is the expected result? -> the builder uses value of the filter, wraps it into the "in" query,
    // then gets whole thing into subquery, and returns the result trough invoking "in" query with the subquery
    $this->exprMock
        ->expects(self::exactly(2))
        ->method('in') // why the in is called two times? -> because first one is a subquery, and second one is a filter.
        ->willReturnCallback(static function (string $field, string $value) use ($subQueryResult, $filterValue, $filterResult, $filterSubQueryResult): string {
            // not same 'a' is returned. and the result is checked in the test.
            if ($field === ReportSubscriber::COMPANY_SEGMENTS_XREF_PREFIX.'.segment_id') {
                // How the expression builder mock corresponds with the incoming data? -> the value gets into the subquery
                self::assertSame($filterValue, $value);

                return $filterSubQueryResult;
            }

            if ($field === ReportSubscriber::COMPANIES_PREFIX.'.id') {
                self::assertSame('('.$subQueryResult.')', $value);

                return $filterResult;
            }

            self::fail('Unknown field: ' . $field);
        });

    $filter = [
        'column'    => 'csx.segment_id',
        'glue'      => 'and',
        'dynamic'   => null,
        'condition' => 'in',
        'value'     => $filterValue,
    ];

    // What are the expectations of this test case? -> Check the filed, field values are correctly placed
    // within the builder and the proper condition is used.
    self::assertSame($filterResult, $this->reportSubscriber->getCompanySegmentCondition($filter));
}

And after seeing this test i will ask in the comment: why you do not use column filter in the method? Thus the bug will be discovered before merging.

[JonasLudwig1998]

Thanks for the example. The tests are updated now. Regarding your column question I'm not 100% sure to which part of the code you are referring to. Could you be a bit more specific? In case you're suggesting that I should consider the case when the column Company Segments is selected as a column to be displayed in the report, there won't be a problem. Company Segments will be only available as filter, not as report data

[biozshock]

@JonasLudwig1998 the column filer is passed in the test case. In the example above it's value is 'csx.segment_id'. The question here is why you do not use the value of the column filter in the method you are testing? For example here: https://github.com/Leuchtfeuer/mautic-CompanySegments-bundle/pull/11/files#diff-8bc8dfe059d8e9152a13b69d1d8617ceb8b3c0a0f9a8e908b88905242c77b178R285

[JonasLudwig1998]

Should be implemented now

[biozshock]

Code looks good. Thanks!