Support pattern matching in tr_link

[cuinixam]

In our project we are using GTest value-parameterized tests - see TEST_P.

Problem: for one parametrized test case, there are multiple test results. With the current implementation of tr_link we can not dynamically link all tests results to the test case itself.

Solution: Update tr_link to use pattern matching if * is contained in the need value.

Bellow is an example of how we use parametrized tests with a copy of tr_link in the conf.py for our project:

// Define a struct to hold the parameters for each test case
struct TestParam {
    const char* description;
    percentage_t mainKnobValue;
    unsigned int expectedBlinkPeriod;
};

// Override the cout operator for TestParam so that it can be printed in the test output
std::ostream& operator<<(std::ostream& os, const TestParam& param)
{
    os << param.description;
    return os;
}

/*!
* @rst
*
* .. test:: BlinkPeriodTests/BlinkPeriodTest.CalculatesCorrectBlinkPeriod/*
*    :id: TS_LC-003
*    :results: [[tr_link('title', 'case')]]
*    :tests: SWDD_LC-002
*
* @endrst
*/
TEST_P(BlinkPeriodTest, CalculatesCorrectBlinkPeriod)
{
    // Get the parameters for this test case
    TestParam param = GetParam();

    unsigned int blinkPeriod = calculateBlinkPeriod(param.mainKnobValue);
    EXPECT_EQ(blinkPeriod, param.expectedBlinkPeriod) << "Test case: " << param.description;
}

// Instantiate the test suite with a set of parameters
INSTANTIATE_TEST_SUITE_P(
    BlinkPeriodTests,
    BlinkPeriodTest,
    ::testing::Values(
        TestParam{ "Slowest", 0, 100 },
        TestParam{ "Inbetween", 50, 50 },
        TestParam{ "Fastest", 100, 10 }
    )
);

The name of a test result in the generated JUnit report follows the following pattern <test suite>/<test class>/<test case>/<test value> and now one can match for <test suite>/<test class>/<test case>/.*

[danwos]

Great work and thanks for the tests. 👍
Maybe we should also add a small hint in the docs as well, that * is supported.

I only have some concerns with the tr_link import in the tests, but see my comments.

And for sure feel free to add yourself to the AUTHORS file, if you like.

[danwos]

This looks like a bug in our CI, if not the locally checked-out package is used, but a version from PyPI.
Sure we really need this?

[danwos]

On my machine a from sphinxcontrib.test_reports.functions import tr_link was enough, after I did:

pip install .
pip install -r test-requirements.txt
pip install sphinxcontrib-plantuml  # a missing dep in the sphinx-needs config

So I guess we do not need this kind of import here.

[cuinixam]

I am using a virtual environment.
I did following:

• create .venv directory in the repository
• run pipenv install -r test-requirements.txt

Executing the tests with pytest fail because sphinx is not installed. So the dependencies in the test-requirements.txt are not enough to execute the tests.

I run pipenv install -r doc-requirements.txt.
All tests now pass.

I removed the ugly import_from_path and just added from sphinxcontrib.test_reports.functions import tr_link.
Now the test_tr_link.py tests are failing because the tr_link from the sphinxcontrib is used. I raised an exception in the local tr_link to make sure this is actually not used. The exception was not raised and the tests are failing.

So my problem is caused by installing the doc dependencies as these contain sphinx-test-reports>=0.3.3.

I am not familiar with nox so I did check why this is not a problem on CI.

There are two things to clarify:

• which steps a developer shall do locally to have a running test environment
• how to make sure that the correct tr_link method is used also when sphinxcontrib.test_reports is available in the Python environment.

[danwos]

Thanks for the insights into your setup, I guess pip install . or similar is missing to install the locally checked-out Sphinx-Test-Report package. Then the correct tr_link should be taken for tests.

[cuinixam]

I have removed the ugly local import hack. Installing the local package with pip install . does the job. I trust that the CI setup will also use the locally checked-out Sphinx-Test-Report package for testing😉
I also briefly mentioned this feature in the docs.

[cuinixam]

I have an issue with getting the pre-commit check to pass. Flake8 complains about the line breaks before binary operators - see W503. In the same time, black formats the code to have the line breaks before binary operators, which I actually prefer. Should I add W503 to the list of ignored warnings in the .flake8 file?

[danwos]

Yes, just add.
I may clean it up one day ;)

[cuinixam]

The pre-commit check passes now on my machine 😀

[cuinixam]

Any chance to review the pr?

[danwos]

Sorry for the annoying delay in my review.

I just found an important problem in the handling of the regex pattern.
See comments below.

This would also be a breaking change, as target_option now gets interpreted as regex pattern, and string-based matches may need an update, if the string contains special regex-chars like . or so.

Maybe it would be a good idea to introduce another option or target_option_regex to activate regex support by the user and to be backward compatible again.

Looking forward on your thoughts about this :)

However, still thanks a lot for the effort you have already spent on this PR 👍

[danwos]

I'm not sure, if this documentation is correct.
target_option is now used as regex-pattern in all cases and sphinxcontrib.test_reports.* works, because of .*, which allows everyhing else after the given string, so also ´sphinxcontrib.test_reportsTHIS_IS_NEW`.

It's not only about the *, but the .*.

Ohh wait, this does not even work with sphinxcontrib.test_reports.*, as . is a special character. . must get escaped: sphinxcontrib\.test_reports\..* .

For some examples see: https://regex101.com/r/XTnHX6/1

Such complex target_option values are not covered by the test cases, as there simpler targets like b are used.

[danwos]

target_option should also contain some more complex regex patterns in some test cases.
For Example:

• package\.module\.function
• package\.module\..* For all functions of a module
• package\.module\.(x|y) Just functions x or y

[cuinixam]

No need to apologise, I totally understand... one cannot handle everything at once and must set priorities 😀
This change is not a blocking point for our projects because I have added the modified tr_link method to conf.py and everything works as expected. Of course, copying the tr_link method in all projects is not a long term solution and therefore I am looking forward to merging this PR 😃

Before I start making any changes, I think we should first define the scope of this PR... like which features will be added for the end user.
The scope initially was to only link one test case to multiple test results by using .* in the target option. That is why the regex matching was only enabled if the target string contained a *.

If we want to offer full regex support, it is a completely different story. I would suggest perhaps adding a tr_link_match for using regular expressions. As you said, changing 'tr_link' to interpret the 'target_option' as a regex pattern is a breaking change. So if we add an option to globally enable regex for the target option, it will imply that existing 'tr_link' may yield a different result.

[danwos]

You are right, The PR gets more complex with every comment. Sorry for that, but I think this is needed to support every aspect a user may see in an option supporting regex or *.

However, keeping tr_link as it is and providing another option like tr_link_match is a good idea.
I would call it tr_link_regex to make clear regex is fully supported here.

If tr_link_regex is set, tr_link is not taken into account anymore.
This should be completely backward compatible.

[cuinixam]

By a tr_link_regex option you mean adding a bool configuration option and if enabled, tr_link will interpret the target_option as a regex pattern? Or do you mean that there should be another method tr_link_regex that the user can use instead of tr_link?

I think I would prefer to add the bool option. The job of tr_link is to link "needs" and you could now configure it to use regular expressions.