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.

Sign up for GitHub

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

Jump to bottom

Add tests #40

Open
wants to merge 11 commits into
base: master
Choose a base branch
from
Open

Add tests #40

wants to merge 11 commits into from

Conversation

blag
Copy link
Contributor

@blag blag commented Mar 30, 2017 •
edited
Loading

Should be applied after/on top of #39. I will rebase this on top of master once that gets merged in.

The first six commits are from #39.

TODO:

  • Fix some bugs uncovered by tests
  • Add tests for email_extras.utils.send_mail function
  • Add tests for the model save() and delete() functions
  • Add tests for the encrypting backend from Add encrypting backend mixin and mix it in with Django's built-in backends #39
  • Add tests BrowsableEmailBackend
  • Add tests for has_add_permission on Address admin
  • Improve coverage for existing test cases (key.email_addresses property)
  • Add tests for signing outgoing mail
  • Add tests for management command
  • Add tests for forms
  • Add tests for default exception handlers, and tests to make sure custom exception handlers are called
  • Figure out what the behavior should be when ALWAYS_TRUST is False and add tests for that - is this even useful? Everything broke when I turned that off... Test for this added from @theithec's comment
  • Add Travis CI integration
  • Add flake8 check
  • Add Travis build status badge and Coveralls coverage status badge to README

There's only one line of crypto code that isn't tested: email_extras/backends.py:72

This PR now has 100% test coverage for django-email-extras!

Further work for future PRs:

  • Add create, update, and delete functions to the queryset on the models so queryset functions operate the same way model functions do
  • Move the save() and delete() functions from the models into their own pre_save, post_save, and pre_delete signal handlers and wire them up in email_extras/apps.py. This should allow us to use serialized fixtures in tests.
  • Make the models swappable

@blag blag force-pushed the add-tests branch 4 times, most recently from 5a068ad to dbc21d3 Compare March 30, 2017 14:25
@theithec
Copy link
Contributor

I think this is really cool. Both, the tests and the backends.
Maybe the tests should live in email_extras, so they would be available to anyone who installed the package. In that case, GNUPG_HOME should be hardcoded in tests.utils and the gpg_keyring directory could move into tests.
And the tests would have to make sure to set email_extras.utils.encrypt_kwargs['always_trust'] is True.

I don't like encrypt_kwargs in settings that much. Maybe utils would be a better place do define it?
And the code duplication in utils and backend should be removed somehow.

Here is a test that could be added:

# tests/utils.py
# import email_extras.utils

def test_send_mail_key_validation_fail_raises_exception(self):
    msg_subject = "Test Subject"
    to = ['[email protected]']
    from_email = settings.DEFAULT_FROM_EMAIL
    msg_text = "Test Body Text"
    msg_html = "<html><body><b>Hello</b> World <i>Text</i>"

    email_extras.utils.encrypt_kwargs['always_trust'] = False
    with self.assertRaises(email_extras.utils.EncryptionFailedError):
        self.send_mail(
            msg_subject, msg_text, from_email, to,
            html_message=mark_safe(msg_html))
    email_extras.utils.encrypt_kwargs['always_trust'] = True

@blag
Copy link
Contributor Author

blag commented Mar 31, 2017 •
edited
Loading

@theithec Thanks for the review. I rewrote the backend (removed admin page and added a management command to generate/upload signing keys), and I'll be rebasing this off of that and adding your test this weekend.

Travis CI results (add-tests branch): Build Status
Coveralls results: Coverage Status

@blag blag force-pushed the add-tests branch 23 times, most recently from c7e1613 to 4df4e04 Compare April 3, 2017 14:38
@blag
Copy link
Contributor Author

blag commented Apr 3, 2017

Aside from getting this reviewed (and any changes that I'll make from that), this is done. It's got 97% overall coverage, and 99% coverage for the crypto code. The only line of crypto code that isn't tested is the second encryption failure test for the backend:

def encrypt(text, addr):
    encryption_result = gpg.encrypt(text, addr, **encrypt_kwargs)
    if not encryption_result.ok:
        raise EncryptionFailedError("Encrypting mail to %s failed: '%s'",
                                    addr, encryption_result.status)
    if smart_text(encryption_result) == "" and text != "":  # <-- THIS LINE
        raise EncryptionFailedError("Encrypting mail to %s failed.",
                                    addr)
    return smart_text(encryption_result)

@blag
Copy link
Contributor Author

blag commented Apr 9, 2017

Rebasing on master now that #41 is merged...

@blag
Copy link
Contributor Author

blag commented Apr 9, 2017

Done rebasing.

@blag blag force-pushed the add-tests branch 4 times, most recently from bae0064 to 1f4c306 Compare April 10, 2017 17:31
@blag blag force-pushed the add-tests branch 2 times, most recently from 585429e to f59ff47 Compare April 10, 2017 17:53
@blag
Copy link
Contributor Author

blag commented Apr 10, 2017 •
edited
Loading

This PR just hit 100% coverage for all code!

I did skip coverage for the following lines. They are either not mission critical (testing body_html and html_message aren't both passed to send_mail, ImportError in email_extras/settings.py), are pretty much guaranteed to work properly (the AppConfig.ready() function being called), or would have permanent, irreversible, global side effects we don't want (gpg.send_keys()).

email_extras/apps.py:11

    def ready(self):  # pragma: noqa

email_extras/management/commands/email_signing_key.py:21

    gpg.send_keys(keyservers, fingerprint)  # pragma: nocover

email_extras/settings.py:31

    except ImportError:  # pragma: no cover

email_extras/utils.py:82,87

    if body_html is not None and html_message is not None:  # pragma: no cover
    if body_html is not None:  # pragma: no cover

Now that this has hit 100%, I'll stop tweaking it. So @theithec if you want to review it again, I would appreciate it. I know that @stephenmcd has a lot of stuff going on at the moment.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@blag @theithec