Test cross contamination in openstack/valiadations-libs

[Jiri Podivin]

The existing tests are using shared fakes as return value
of certain mocks.

Unfortunately, while perfectly fine in most cases,
certain methods change values passed in place.
This created a race condition, which caused fluctuation in the reported
coverage of the relevant modules.

The undesirable test behavior only becomes apparent when the tests
are run in a succession and their behavior is analyzed, for example
during the run of the recently implemented 'coverchange' job.

Final statistics of the coverage measurement, obtained for the openstack/validations-libs on master branch at commit 85947ee8e0e6c04f5b0bbf5fd559d1fbcac8d6c5,
displaying the totals for number of lines, lines without coverage, branches etc.,
display following pattern. Note the third column, indicating number of lines without coverage.

------------------------------------------------------------
TOTAL 995 105 340 56 86%
TOTAL 995 105 340 56 86%
TOTAL 995 105 340 56 86%
TOTAL 995 108 340 56 86%
TOTAL 995 108 340 56 86%
TOTAL 995 105 340 56 86%
TOTAL 995 105 340 56 86%
TOTAL 995 108 340 56 86%
TOTAL 995 105 340 56 86%
-------------------------------------------------------------

Number of lines without coverage oscillates between '108' and '105' depending
on the order in which the tests were run. The culprit being one of the functions
of the 'validations_libs.cli.common' module, which edits data passed in place.

Code snip:
~~~~~~~~~~
for host in row['Status_by_Host'].split(', '):
    try:
        _name, _status = host.split(',')
    except ValueError:
        # if ValueError, then host is in unknown state:
        _name = host
        _status = 'UNKNOWN'
    _name = colors.color_output(_name, status=_status)
    hosts.append(_name)

As the data processed are passed from patched object, and the data itself are defined withing the shared 'fakes' module, it emerges with inevitability, that the tested code operates on data previously altered by the tests.

[OpenStack Infra (hudson-openstack)]

Reviewed: https://review.opendev.org/c/openstack/validations-libs/+/799754
Committed: https://opendev.org/openstack/validations-libs/commit/b2a9d713612d8ef0b4058347fc37b4a4c6e80ec2
Submitter: "Zuul (22348)"
Branch: master

commit b2a9d713612d8ef0b4058347fc37b4a4c6e80ec2
Author: Jiri Podivin <email address hidden>
Date: Wed Jul 7 10:24:53 2021 +0200

    Test cross contamination prevention

    The existing tests were using shared fakes as return value
    of certain mocks. Unfortunately, while perfectly fine in most cases,
    certain methods change values passed in place.
    This created a race condition, which caused fluctuation in the reported
    coverage of the relevant modules.

    The undesirable test behavior only becomes apparent when the tests
    are run in a succession and their behavior is analyzed, for example
    during the run of the recently implemented 'coverchange' job.

    Final statistics of the coverage measurement, displaying the totals
    for number of lines, lines without coverage, branches etc., display
    following pattern. Note the third column.

    TOTAL 995 105 340 56 86%
    TOTAL 995 105 340 56 86%
    TOTAL 995 105 340 56 86%
    TOTAL 995 108 340 56 86%
    TOTAL 995 108 340 56 86%
    TOTAL 995 105 340 56 86%
    TOTAL 995 105 340 56 86%
    TOTAL 995 108 340 56 86%
    TOTAL 995 105 340 56 86%

    Number of lines without coverage oscillates between '108' and '105' depending
    on the order in which the tests were run. The culprit being one of the functions
    of the 'validations_libs.cli.common' module, which edits data passed in place.

    As the data processed are passed from patched object, and the data itself are
    defined withing the relevant 'fakes' module, it emerges with inevitability,
    that the tested code operates on data previously altered by the tests.

    Furthermore the 'test_run_command_failed_validation' test was augmented
    to place further assertion on the arguments passed.

    Closes-Bug: #1935003

    Signed-off-by: Jiri Podivin <email address hidden>
    Change-Id: I2210fa1775691dd503eb882a24824eaf18d7bd3e

[OpenStack Infra (hudson-openstack)]

This issue was fixed in the openstack/validations-libs 1.3.0 release.