Enhancement: Refactor Test Code for Efficiency and Quality

[freddiewanah]

Description:

As a first-time contributor with a background in researching Python unit tests, I conducted a thorough static analysis of the project's test code. This examination revealed various "test smells" that could potentially degrade both the efficiency and quality of our tests. To address these issues, I've refactored the relevant code segments. The modifications I propose not only aim to eliminate these inefficiencies but are also expected to reduce execution time within GitHub Actions. This reduction could contribute to lower operational costs for the project.

Test smells refactored:

• Suboptimal Assert: Using assert XXX is True. The is True check is redundant. This will decrease the code overall readability and increase the execution time as extra logic needed.
• Exception Handling: Instead of catching exceptions with assertRaise, use try/except commands. This will cause extra execution time according to the experimental tests and didn't use the benefits of the test frameworks.
• Magic Number: occurs when assert statements in a test case contain numeric literals (i.e., magic numbers) as parameters instead of more descriptive constants or variables

I didn't change any of the testing logic or testing values. The refactoring only focuses on reducing the test smells.

Please feel free to let me know if you are interested in more refactoring like this or if there're any changes that you don't wish.

Checklist

• [x] The code change is tested and works locally.
• [x] There is no commented out code in this PR.
• [x] No lint errors (use flake8)
• [x] 100% test coverage

Testing

Anyone can help test this source code as follows:

# Create a virtual environment to work in as follows:
python3 -m venv apprise

# Change into our new directory
cd apprise

# Activate our virtual environment
source bin/activate

# Install the branch
pip install git+https://github.com/freddiewanah/apprise.git

[caronc]

I totally support the with pytest.raises() changes; those are great. 🚀🙂

However, i disagree with the removal of is True everywhere. Anything that isn't None, False or doesn't have a __class__.__bool__() defined will always pass this condition with your new change.

# consider that all of these statements are True:
assert 1
assert 2
assert 'hello' 
assert True
assert object()
assert 0.00001

By removing the is True, you're removing the check of additional verifying the returned object itself (you reduce the scope of expression being validated)

# The following checks 2 very different distinct things 
assert (1 + 1)
assert (1 + 1) == 2

[freddiewanah]

Thank you so much for the feedback, I'm sorry for being reckless and removed all the is True check. I will go over and revise the corresponding code.

[caronc]

No need to appologize at all. I appreciate your interest in trying to improve the code base! 🏆

[freddiewanah]

Hi, I've reverted most of the assert xxx is True, but for statements like isInstance or startswith, I think it's find to remove the is True check.

Another thing I noticed is some of the test cases are too big, with duplicated asserts repeated in one test case (e.g., test_plugin_dbus_general_success). Do you want me to refactor it with the @pytest.parameterize to make the code clean and also efficient to execute?

[caronc]

That's totally up to you. Consider the some tests appear as though they repeat, but they actually test sections of the code that previous calls created a cache entry for. So the additional calls test the handling of cached responses.

Apprise has 100% test coverage except for the NotifyDBus section which needs a complete refactoring. Another kind person already started a PR on this already... The point is that if you start cleaning up and refactoring tests; outside of the above Service/Plugin, there should be no change in code coverage. 😉

[freddiewanah]

Thanks. I will see how I can refactor some of the cases and make sure that won't affect the test coverage and the basic test logic :)

[freddiewanah]

It has been some time since our last discussion on this PR. Regarding the test style implemented here, I'm unsure if it aligns with the project's preferences and would appreciate your feedback. If this approach meets your expectations, I'm ready to extend these refactorings further.

For instance, in the test_is_email, currently, if one assertion fails, the subsequent tests do not execute, potentially introducing additional issues. I propose adopting a pytest.mark.parametrize approach to enhance the robustness of our test suite. Below is my proposed methodology for your review.

@pytest.mark.parametrize(
    "input_email, expected",
    [
        ('test@gmail.com', {'name': '', 'email': 'test@gmail.com', 'full_email': 'test@gmail.com', 'domain': 'gmail.com', 'user': 'test', 'label': ''}),
        ('test@my-valid_host.com', {'name': '', 'email': 'test@my-valid_host.com', 'full_email': 'test@my-valid_host.com', 'domain': 'my-valid_host.com', 'user': 'test', 'label': ''}),
        ('tag+test@gmail.com', {'name': '', 'email': 'test@gmail.com', 'full_email': 'tag+test@gmail.com', 'domain': 'gmail.com', 'user': 'test', 'label': 'tag'}),
        ('Bill Gates: bgates@microsoft.com', {'name': 'Bill Gates', 'email': 'bgates@microsoft.com', 'full_email': 'bgates@microsoft.com', 'domain': 'microsoft.com', 'user': 'bgates', 'label': ''}),
        ('Bill Gates <bgates@microsoft.com>', {'name': 'Bill Gates', 'email': 'bgates@microsoft.com', 'full_email': 'bgates@microsoft.com', 'domain': 'microsoft.com', 'user': 'bgates', 'label': ''}),
        ('Bill Gates: <bgates@microsoft.com>', {'name': 'Bill Gates', 'email': 'bgates@microsoft.com', 'full_email': 'bgates@microsoft.com', 'domain': 'microsoft.com', 'user': 'bgates', 'label': ''}),
        ('Sundar Pichai <ceo+spichai@gmail.com>', {'name': 'Sundar Pichai', 'email': 'spichai@gmail.com', 'full_email': 'ceo+spichai@gmail.com', 'domain': 'gmail.com', 'user': 'spichai', 'label': 'ceo'}),
        ('"Chris Hemsworth" <ch@test.com>', {'name': 'Chris Hemsworth', 'email': 'ch@test.com', 'full_email': 'ch@test.com', 'domain': 'test.com', 'user': 'ch', 'label': ''}),
        ('      <spichai@gmail.com>', {'name': '', 'email': 'spichai@gmail.com', 'full_email': 'spichai@gmail.com', 'domain': 'gmail.com', 'user': 'spichai', 'label': ''}),
        ("Name valid@example.com", {'name': 'Name', 'email': 'valid@example.com', 'full_email': 'valid@example.com', 'domain': 'example.com', 'user': 'valid', 'label': ''}),
        ("Руслан Эра russian+russia@example.ru", {'name': 'Руслан Эра', 'email': 'russia@example.ru', 'full_email': 'russian+russia@example.ru', 'domain': 'example.ru', 'user': 'russia', 'label': 'russian'}),
        ('invalid.com', False),
        (object(), False),
        (None, False),
        ("Just A Name", False),
        ("Name <bademail>", False),
        ('a-z0-9_!#$%&*+/=?%`{|}~^.-@gmail.com', {'name': '', 'email': '/=?%`{|}~^.-@gmail.com', 'full_email': 'a-z0-9_!#$%&*+/=?%`{|}~^.-@gmail.com', 'domain': 'gmail.com', 'user': '/=?%`{|}~^.-', 'label': 'a-z0-9_!#$%&*'}),
        ('a-z0-9_!#$%&*/=?%`{|}~^.-@gmail.com', {'name': '', 'email': 'a-z0-9_!#$%&*/=?%`{|}~^.-@gmail.com', 'full_email': 'a-z0-9_!#$%&*/=?%`{|}~^.-@gmail.com', 'domain': 'gmail.com', 'user': 'a-z0-9_!#$%&*/=?%`{|}~^.-', 'label': ''}),
    ]
)
def test_is_emaill(input_email, expected):
    assert utils.is_email(input_email) == expected

Maybe I should submit a new PR for such changes?

[caronc]

@freddiewanah I don't mind merging what you have if you want. I'm not sure if you're not just giving yourself extra work to do your suggested changes. But if you see value in it (and the code coverage doesn't suffer), by all means go for it. :rocket:

[freddiewanah]

Thanks @caronc . Please proceed with merging this one.

It may take me a while to complete and create a new PR, but I'm willing to spend some time working on this.

[caronc]

Thank you for all of your great work!

[dgtlmoon]

great PR, nice work!