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.

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

Issue 54 add tests to django example app #434

Merged
merged 17 commits into from
Mar 13, 2020
Merged

Issue 54 add tests to django example app #434

merged 17 commits into from
Mar 13, 2020

Conversation

sichgeis
Copy link
Contributor

@sichgeis sichgeis commented Feb 1, 2020

Views, models and tests have been included into the example app. These follow the tutorial in the Django documentation.

Description

I would like to contribute to this project and picked the issue #54 to implement. I extended the example app with views, models and tests.

This relates to the issue "Add tests to django example app" #54

The example app is for displaying and voting on polls. This is follows the official Django tutorial from Django 2.2 that can be found here: https://docs.djangoproject.com/en/2.2/intro/tutorial01/

I have tested the changes to the project with a fresh install following the README.md and checked that installation, build and running is ok.

Motivation and Context

I have created views, models and corresponding tests to showcase a typlical Django app.

Screenshots (if appropriate):

Steps to reproduce (if appropriate):

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code follows the code style of this project.
  • My change requires documentation updates.
  • I have updated the documentation accordingly.
  • My change requires dependencies updates.
  • I have updated the dependencies accordingly.

@jhgv jhgv self-requested a review February 4, 2020 12:40
Copy link
Contributor

@jhgv jhgv left a comment

Choose a reason for hiding this comment

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

Hi @hansdereber, thanks for contributing and nice work!

I have just a few comments about your changes.

from django.test import TestCase
from django.utils import timezone

from ..models import Question
Copy link
Contributor

Choose a reason for hiding this comment

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

Please use the full module name here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

from django.urls import reverse
from django.utils import timezone

from ..models import Question
Copy link
Contributor

Choose a reason for hiding this comment

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

Please use the full module name here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

README.md Outdated
@@ -102,6 +103,8 @@ Add the libname to either requirements.in or dev-requirents.in, then either upgr

### Cleaning example code
Before start creating your own apps, run the command `make clean_examples` in order to clean up the example apps from the front and backend.
Deregister the example app by removing `'exampleapp.apps.ExampleappConfig'` from ``{{project_name}}/settings/base.py`` and adjust
Copy link
Contributor

Choose a reason for hiding this comment

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

This step would be better to be done before the command make clean_examples. Just to avoid getting errors after removing the code. Please also mention that the example app URLs should be removed from {{project_name}}/urls.py

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@sichgeis
Copy link
Contributor Author

@jhgv thanks for the quick reply! I have made the changes to the module import and the README as suggested.

Also I needed to change the circle ci config.yaml file to make the django unit tests work in the ci. The issue was, that the tests for the views require the webpack-stats.json file to be build by "npm build". For this i have reordered the config.yaml file. It now builds and tests the frontend first, then it builds and tests the backend. Now the ci build is green with all 10 tests passing.

Kindly check the PR again.

@sichgeis sichgeis requested a review from jhgv February 11, 2020 19:47
@sichgeis
Copy link
Contributor Author

@jhgv : could you take a look at the changes in this pr. If this is too big, i can remove 4 tests which then avoids touching the circleci change. But imho the proposed change in the circleci config makes sense. What do you think?

@jhgv
Copy link
Contributor

jhgv commented Feb 20, 2020

Hi @hansdereber! Sorry for the delay, it's been a tough last two weeks.

Everything looks great but I have one more concern. Before removing the example code, it will be nice to remove the exampleapp tables that were generated.

It is possible with the command:
python manage.py migrate exampleapp zero
and it can be placed in the Makefile (first command before removing the app)

What do you think?

@sichgeis
Copy link
Contributor Author

@jhgv removing the tables is a good idea 👍 I have added a single line in the Makefile as suggested and adapted the README as well. The steps in the README are now with bulletpoints which looks cleaner.

I did a full smoke test on the creation and removal of the sample app and I am confident that everything works as expected. Kindly check the updated pr.

ps: It turns out that you cannot deregister the exampleapp from the INSTALLED_APPS list before running the 'zero' command. Therefore the order of steps to remove the sample app has changed in the README.

@sichgeis
Copy link
Contributor Author

sichgeis commented Mar 9, 2020

Hey @jhgv, bump for this PR.

@jhgv
Copy link
Contributor

jhgv commented Mar 13, 2020

LGTM ✔️ 📦 ! Thank you for contributing!

@jhgv jhgv merged commit b4b5dd6 into vintasoftware:master Mar 13, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants