Skip to content
This repository has been archived by the owner on Sep 10, 2020. It is now read-only.

[QA] Increasing test coverage to 100% #186 #202

Merged
merged 1 commit into from
Nov 18, 2018
Merged

[QA] Increasing test coverage to 100% #186 #202

merged 1 commit into from
Nov 18, 2018

Conversation

strang1ato
Copy link
Contributor

@strang1ato strang1ato changed the title [QA] Increasing test coverage to 100% #186 [WIP] [QA] Increasing test coverage to 100% #186 Nov 13, 2018
@strang1ato
Copy link
Contributor Author

I stuck while testing these lines (I tried to test it in test_admin.py in my PR) can you give some hints how test this function?

@coveralls
Copy link

Coverage Status

Coverage increased (+2.05%) to 99.427% when pulling 7f0c69c on OltarzewskiK:issues/186 into abbe3bc on openwisp:master.

@coveralls
Copy link

coveralls commented Nov 13, 2018 •
edited
Loading

Coverage Status

Coverage increased (+1.6%) to 100.0% when pulling 49fb856 on OltarzewskiK:issues/186 into 3d826ae on openwisp:master.

@strang1ato
Copy link
Contributor Author

I have tried to test these lines by that:

    def test_save_radiuscheck(self):
        obj = self._create_radius_check(**_RADCHECK_ENTRY)
        self.client.post(reverse('admin:{0}_radiuscheck_change'.format(self.app_name),
                                            args=[obj.pk]), {'username': 'Monica', 'new_value': 'Cam0_liXX',
                             'attribute': 'NT-Password', 'op': ':='}, follow=True)
        response = self.client.get(reverse('admin:{0}_radiuscheck_changelist'.format(self.app_name)))
        self.assertContains(response, '6c98504d6aefc23127b2bb64578d3c2d')

but by client.post I can only test status_code so it doesn't work and somebody have tried to test this line too here.

And I can not figure out where this function is used, so I added for both pragma nocove

@strang1ato strang1ato changed the title [WIP] [QA] Increasing test coverage to 100% #186 [QA] Increasing test coverage to 100% #186 Nov 14, 2018
@nemesifier nemesifier mentioned this pull request Nov 16, 2018
Merged
nemesifier
nemesifier suggested changes Nov 16, 2018
View reviewed changes
django_freeradius/base/models.py Outdated
@@ -206,7 +206,7 @@ class AbstractRadiusCheckManager(models.Manager):
def get_queryset(self):
return AbstractRadiusCheckQueryset(self.model, using=self._db)

def create(self, *args, **kwargs):
def create(self, *args, **kwargs): # pragma nocover
Copy link
Member

Choose a reason for hiding this comment

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

you can test this with RadiusCheck.objects.create() and supply new_value in the keyword arguments

django_freeradius/tests/base/test_admin.py Outdated
@@ -187,6 +187,7 @@ def test_radiuscheck_filter_expired(self):
def test_radiuscheck_filter_not_expired(self):
url = reverse('admin:{0}_radiuscheck_changelist'.format(self.app_name)) + '?expired=not_expired'
resp = self.client.get(url, follow=True)
# print(resp.context)
Copy link
Member

Choose a reason for hiding this comment

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

remove this line

django_freeradius/base/admin.py Outdated
@@ -83,7 +83,7 @@ class AbstractRadiusCheckAdmin(TimeStampedEditableAdmin):
autocomplete_fields = ['user']
actions = [disable_action, enable_action, delete_selected]

def save_model(self, request, obj, form, change):
def save_model(self, request, obj, form, change): # pragma: nocover
Copy link
Member

Choose a reason for hiding this comment

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

remove # pragma: nocover. Never use this for real code but only for exceptional and special cases.
I will test this method and let you know what I find.

Copy link
Member

Choose a reason for hiding this comment

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

#205

nemesifier
nemesifier reviewed Nov 16, 2018
View reviewed changes
Copy link
Member

@nemesifier nemesifier left a comment

Choose a reason for hiding this comment

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

@OltarzewskiK rebase on the current master to fix the issue of save_model

@strang1ato strang1ato changed the title [QA] Increasing test coverage to 100% #186 [WIP] [QA] Increasing test coverage to 100% #186 Nov 17, 2018
@EdgeKing810
Copy link
Contributor

You're good @OltarzewskiK
Great job man!

@strang1ato
Copy link
Contributor Author

Thanks 😊

nemesifier
nemesifier suggested changes Nov 17, 2018
View reviewed changes
Copy link
Member

@nemesifier nemesifier left a comment

Choose a reason for hiding this comment

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

Great progress Karol!

SAMPLE_APP=1 runs tests for the sample_app in 'tests/` to test custom models using the swappable base models, you can run tests for that as well with:

SAMPLE_APP=1 ./runtests.py --keepdb

The keepdb flag just speeds up test execution by avoiding to recreate the test db.

django_freeradius/tests/base/test_models.py Outdated
@@ -6,6 +6,7 @@
DEFAULT_SESSION_TIME_LIMIT, DEFAULT_SESSION_TRAFFIC_LIMIT, SESSION_TIME_ATTRIBUTE,
SESSION_TRAFFIC_ATTRIBUTE,
)
from django_freeradius.models import RadiusCheck
Copy link
Member

Choose a reason for hiding this comment

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

this is what is causing the issue, these tests should not import concrete models because are designed to be reused (and are reused in the sample app).

Read the code of other tests to see how they get the model class: https://github.com/openwisp/django-freeradius/blob/master/django_freeradius/tests/base/test_models.py#L25
You should always do this when in doubt: read how existing code does something.

@strang1ato strang1ato changed the title [WIP] [QA] Increasing test coverage to 100% #186 [QA] Increasing test coverage to 100% #186 Nov 18, 2018
nemesifier
nemesifier approved these changes Nov 18, 2018
View reviewed changes
Copy link
Member

@nemesifier nemesifier left a comment

Choose a reason for hiding this comment

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

Yes 👍

@nemesifier nemesifier merged commit aa1ceb6 into openwisp:master Nov 18, 2018
@strang1ato strang1ato deleted the issues/186 branch November 18, 2018 11:58
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@nemesifier nemesifier nemesifier approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@strang1ato @coveralls @EdgeKing810 @nemesifier