Skip to content

Change Django Tests to pytests - #133

Merged
haikoschol merged 1 commit into
aboutcode-org:developfrom
sbs2001:develop
Dec 19, 2019
Merged

Change Django Tests to pytests#133
haikoschol merged 1 commit into
aboutcode-org:developfrom
sbs2001:develop

Conversation

@sbs2001

@sbs2001 sbs2001 commented Dec 14, 2019

Copy link
Copy Markdown
Collaborator

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
Copy Markdown
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.

Comment thread vulnerabilities/tests/test_data_dump.py Outdated
Comment thread vulnerabilities/tests/test_api.py Outdated
@sbs2001

sbs2001 commented Dec 16, 2019

Copy link
Copy Markdown
Collaborator Author

@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!

@sbs2001
sbs2001 requested a review from haikoschol December 17, 2019 08:05
@haikoschol

Copy link
Copy Markdown
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.

Comment thread vulnerabilities/tests/conftest.py Outdated
@sbs2001
sbs2001 marked this pull request as ready for review December 19, 2019 06:45
@sbs2001
sbs2001 requested a review from haikoschol December 19, 2019 06:53
Signed-off-by: sbs2001 <shivam.sandbhor@gmail.com>

@haikoschol haikoschol left a comment

Copy link
Copy Markdown
Collaborator

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