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

Change Django Tests to pytests #133

Merged
merged 1 commit into from
Dec 19, 2019
Merged

Change Django Tests to pytests #133

merged 1 commit into from
Dec 19, 2019

Conversation

sbs2001
Copy link
Collaborator

@sbs2001 sbs2001 commented Dec 14, 2019

Signed-off-by: sbs2001 shivam.sandbhor@gmail.com

I have used this pytest extension. We are able to run all the tests simply by running pytest in the root directory.

I am currently working on fixing the following warnings(I could use some help here).

(venv) shivam@shivam-Latitude-D630:~/coding/opensource/vulnerablecode$ pytest
===================================================================== test session starts =====================================================================
platform linux -- Python 3.6.9, pytest-5.3.2, py-1.8.0, pluggy-0.13.1
Django settings: vulnerablecode.settings (from ini file)
rootdir: /home/shivam/coding/opensource/vulnerablecode, inifile: pytest.ini
plugins: django-3.7.0
collected 32 items                                                                                                                                            

vulnerabilities/tests/test_api.py ......                                                                                                                [ 18%]
vulnerabilities/tests/test_data_dump.py ............                                                                                                    [ 56%]
vulnerabilities/tests/test_models.py ....                                                                                                               [ 68%]
vulnerabilities/tests/test_api_data.py ..                                                                                                               [ 75%]
vulnerabilities/tests/test_import_cli.py ...                                                                                                            [ 84%]
vulnerabilities/tests/test_npm.py ...                                                                                                                   [ 93%]
vulnerabilities/tests/test_scrapers.py ..                                                                                                               [100%]

====================================================================== warnings summary =======================================================================
vulnerabilities/tests/test_api.py::test_debian_query_by_name
vulnerabilities/tests/test_api.py::test_debian_query_by_invalid_package_url
vulnerabilities/tests/test_api.py::test_debian_query_by_package_url
vulnerabilities/tests/test_api.py::test_debian_query_by_package_url_without_namespace
vulnerabilities/tests/test_api.py::test_ubuntu_response
  /home/shivam/coding/opensource/vulnerablecode/venv/lib/python3.6/site-packages/whitenoise/base.py:116: UserWarning: No directory at: /home/shivam/coding/opensource/vulnerablecode/staticfiles/
    warnings.warn(u"No directory at: {}".format(root))

vulnerabilities/tests/test_api.py::test_debian_query_by_name
vulnerabilities/tests/test_api.py::test_ubuntu_response
  /home/shivam/coding/opensource/vulnerablecode/venv/lib/python3.6/site-packages/django_filters/rest_framework/backends.py:31: MigrationNotice: `PackageViewSet.filter_fields` attribute should be renamed `filterset_fields`. See: https://django-filter.readthedocs.io/en/master/guide/migration.html
    filterset_class = self.get_filterset_class(view, queryset)

-- Docs: https://docs.pytest.org/en/latest/warnings.html
========================================================== 32 passed, 7 warnings in 88.52s (0:01:28) ==========================================================

@haikoschol haikoschol self-requested a review December 15, 2019 11:45
@haikoschol
Copy link
Collaborator

@sbs2001 Thanks for this PR!

Since all tests are now being discovered and run with one command, can you also remove the old call to pytest from .travis.yml?

I think the warnings are unrelated to your changes. My guess is that pytest shows them by default whereas the Django test runner doesn't. Two dependencies (whitenoise and Django REST Framework) use features of Django that will be removed in a future version. It is possible that there are more recent versions of those apps that address these warnings. You can try changing their versions in requirements.txt to the latest to check.

In the chat you wrote:

I am bit shaky about whether I used the fixtures and marks effectively.

I think you use them correctly. However, the marks are only used in test_models.py which does not actually test any behaviour of vulnerablecode but just basic Django ORM operations. I don't think having these tests provides any benefit so I deleted the file in my pending PR #132.

And regarding the fixtures; Having them in a separate file increases the distance to the code using them and arguably makes it harder to read the tests. @pombredanne commented elsewhere that he prefers tests to be self-contained if possible. Personally I don't mind it and prefer to avoid duplication if that is the tradeoff to be made. Maybe @pombredanne wants to chime in on this? Otherwise, if we do leave it as is, would you mind renaming the file from conftest.py to fixtures.py?

I didn't expect you to change all the existing tests from TestCase subclasses to simple functions. But since you did I'm fine with keeping them that way. However by removing the extra namespace of the class, you created a few name clashes, effectively removing some tests. If you look at the CI log on your PR, it says 27 passed after running pytest. Before, running manage.py test, it reported 28 tests. The four tests that needed to be run separately before are now included in running pytest, so the total number of tests should be 32.

@sbs2001
Copy link
Collaborator Author

sbs2001 commented Dec 16, 2019

@haikoschol I have corrected the code. I changed the names to avoid name clashes. The warnings are not fixed though. Regarding conftest.py , after researching albeit I don't think we can change that to fixtures.py . I used a separate file because we need the same fixtures in multiple test files.Thanks for the detailed review!

@haikoschol
Copy link
Collaborator

The warnings are not fixed though.

We can look into that after merging.

Regarding conftest.py , after researching albeit I don't think we can change that to fixtures.py .

I didn't realize that the filename has special meaning for pytest. In that case we'll leave it as is.

vulnerabilities/tests/conftest.py Outdated Show resolved Hide resolved
@sbs2001 sbs2001 marked this pull request as ready for review December 19, 2019 06:45
Signed-off-by: sbs2001 <shivam.sandbhor@gmail.com>
Copy link
Collaborator

@haikoschol haikoschol left a comment

Choose a reason for hiding this comment

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

Looks good now. @sbs2001 thanks again for contributing!

@haikoschol haikoschol merged commit 9909f7e into aboutcode-org:develop Dec 19, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants