Enhancement: Refactor Test Code for Efficiency and Quality by freddiewanah · Pull Request #1100 · caronc/apprise

freddiewanah · 2024-04-09T15:54:13Z

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

The code change is tested and works locally.
There is no commented out code in this PR.
No lint errors (use flake8)
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 · 2024-04-10T10:56:13Z

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 · 2024-04-10T14:11:13Z

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 · 2024-04-10T19:42:53Z

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

freddiewanah · 2024-04-11T08:52:44Z

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 · 2024-04-11T13:37:17Z

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 · 2024-04-11T14:15:03Z

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 · 2024-04-19T15:41:58Z

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 · 2024-04-20T19:59:20Z

@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. 🚀

freddiewanah · 2024-04-20T20:29:19Z

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 · 2024-04-20T23:00:50Z

Thank you for all of your great work!

dgtlmoon · 2024-05-12T12:44:16Z

great PR, nice work!

freddiewanah added 5 commits April 8, 2024 15:51

refactor test methods to avoid exception handling.

e0d3741

refactor the magic number smell

f57129c

refactored the suboptimal assert and missing asserts

44afce2

Merge branch 'caronc:master' into master

d578c57

update code style

7a3793a

freddiewanah added 2 commits April 11, 2024 10:39

revert part of the is True check

08313f2

fix missing asserts

d78daa8

caronc merged commit 08cb018 into caronc:master Apr 20, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Enhancement: Refactor Test Code for Efficiency and Quality#1100

Enhancement: Refactor Test Code for Efficiency and Quality#1100
caronc merged 7 commits intocaronc:masterfrom
freddiewanah:master

freddiewanah commented Apr 9, 2024 •

edited by caronc

Loading

Uh oh!

caronc commented Apr 10, 2024 •

edited

Loading

Uh oh!

freddiewanah commented Apr 10, 2024

Uh oh!

caronc commented Apr 10, 2024

Uh oh!

freddiewanah commented Apr 11, 2024

Uh oh!

caronc commented Apr 11, 2024 •

edited

Loading

Uh oh!

freddiewanah commented Apr 11, 2024

Uh oh!

freddiewanah commented Apr 19, 2024

Uh oh!

caronc commented Apr 20, 2024

Uh oh!

freddiewanah commented Apr 20, 2024

Uh oh!

caronc commented Apr 20, 2024

Uh oh!

dgtlmoon commented May 12, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

freddiewanah commented Apr 9, 2024 • edited by caronc Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description:

Checklist

Testing

Uh oh!

caronc commented Apr 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

freddiewanah commented Apr 10, 2024

Uh oh!

caronc commented Apr 10, 2024

Uh oh!

freddiewanah commented Apr 11, 2024

Uh oh!

caronc commented Apr 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

freddiewanah commented Apr 11, 2024

Uh oh!

freddiewanah commented Apr 19, 2024

Uh oh!

caronc commented Apr 20, 2024

Uh oh!

freddiewanah commented Apr 20, 2024

Uh oh!

caronc commented Apr 20, 2024

Uh oh!

dgtlmoon commented May 12, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

freddiewanah commented Apr 9, 2024 •

edited by caronc

Loading

caronc commented Apr 10, 2024 •

edited

Loading

caronc commented Apr 11, 2024 •

edited

Loading