Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Avoid modifying global category/filters in tests #6503

Merged
merged 1 commit into from
Oct 28, 2021

Conversation

devos50
Copy link
Contributor

@devos50 devos50 commented Oct 27, 2021

In the metadata store tests, we are modifying default_xxx_filter and add a term to it. This change is persistent across different tests, potentially leading to test failures. This PR modifies the tests and increases flexibility by creating separate instances of Category and XXXFilter. These objects are manipulated without affecting default_category_filter or default_xxx_filter.

Fixes #6500 (hopefully)

@devos50 devos50 force-pushed the tests_improvements branch from 8a574e1 to 2681bcb Compare October 27, 2021 08:28
@devos50 devos50 marked this pull request as ready for review October 27, 2021 08:43
@devos50 devos50 requested review from a team and kozlovsky and removed request for a team October 27, 2021 08:43
Copy link
Contributor

@kozlovsky kozlovsky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! It is better to have tests isolated by avoiding of global variables.

I think the actual reason for the error in #6500 was this line:

async def test_get_my_channel_tags_xxx(metadata_store, tags_db, mock_dlmgr_get_download, my_channel, rest_api):  # pylint: disable=redefined-outer-name
    """
    Test whether XXX tags are correctly filtered
    """
    with db_session:
        ...
        default_xxx_filter.xxx_terms = {"wrongterm"}

If it was executed before tests in test_category.py it could destroy the expected default_xxx_filter content.

I think it should be fixed as well (in this or separate PR)

@kozlovsky kozlovsky self-requested a review October 27, 2021 13:27
kozlovsky
kozlovsky previously approved these changes Oct 27, 2021
@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@devos50
Copy link
Contributor Author

devos50 commented Oct 28, 2021

@kozlovsky thanks for pointing that out! I tried to see if there is an easy way to get rid of the default_category_filter and default_xxx_filter variables and inject them in the components. This way, we should also be able to replace entries in the filter during the tests. This, however, turned out to be much work than I anticipated since the category filter is integrated in the metadata store and not easy to replace on runtime.

We could consider making a component out of the category filter and pass it to the components that need it (e.g., REST endpoints/metadata store). I'm not sure how much work that is but this is more something for a subsequent PR.

@devos50 devos50 requested a review from kozlovsky October 28, 2021 10:36
@devos50 devos50 merged commit fd4bea5 into Tribler:main Oct 28, 2021
@devos50 devos50 deleted the tests_improvements branch October 28, 2021 12:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

AssertionError: assert 'other' == 'xxx' (in tests)
3 participants