Skip to content
This repository has been archived by the owner on Jan 26, 2021. It is now read-only.

New unit tests for event #36

Merged
merged 2 commits into from
Jun 12, 2015
Merged

New unit tests for event #36

merged 2 commits into from
Jun 12, 2015

Conversation

vubo
Copy link
Contributor

@vubo vubo commented Jun 7, 2015

There are new unit tests for all functions in event: event_not_empty, delete_event, get_event_by_id, get_events_ordered_by_name.

I also created a new function event_not_empty in event/services.py based on the #34 and #28 (comment).

@vubo vubo mentioned this pull request Jun 7, 2015
5 tasks
if not event:
result = False
return result

#need to check that this event is not accociated with any jobs,
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's remove these comments. It would be better to add a docstring for the test. There should be examples in the Django docs on docstrings.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@willingc yes, I found it: https://docs.djangoproject.com/en/1.5/topics/testing/doctests/
Do you mean the whole function event_not_empty or only # comments ?

Copy link
Contributor

Choose a reason for hiding this comment

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

@vubo Just the comments please :) As an FYI, it's probably a good idea to use the Django 1.8 docs https://docs.djangoproject.com/en/1.8/ as the primary resource.

For example,

def event_not_empty(event_id):
    """ Returns true if an event exists and is not empty """

We may wish to revisit the naming of these functions at a later date to improve understanding simply from the function names and it may decrease the number of not statements and double negation. For now, don't worry about it. I'll need to have some additional time to think on this.

Thanks for researching the issue and asking for clarification :)

Copy link
Contributor

Choose a reason for hiding this comment

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

@vubo Just an FYI, I prefer one line docstrings if the function is straightforward/easily understandable. If it is a complicated function with lots of passed parameters then please use a multi-line docstring. In general, I prefer self documenting code -- less comments and better names :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@willingc thank you, I will do it. I was confused about this:
"As with unit tests, for a given Django application, the test runner looks for doctests in two places: The models.py file and tests.py" (https://docs.djangoproject.com/en/1.5/topics/testing/doctests/)
And there is a file services.py

Copy link
Contributor

Choose a reason for hiding this comment

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

@vubo For now and likely the future, we will only use the docstrings for documentation. We will not use it for the testing function mentioned in your quote above. I'm not a huge fan of doctests ;)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@willingc Ok, I get it :)

@willingc
Copy link
Contributor

Tests pass cleanly. I am going to merge this PR for initial unit tests for event app. These tests will be refactored in a similar fashion as Issue #41.

@vubo Happy to merge 💐

willingc added a commit that referenced this pull request Jun 12, 2015
Add new unit tests for event app
@willingc willingc merged commit af4fc9a into anitab-org:develop Jun 12, 2015
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants