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

Correct API docs path and fix pytest invocation #379

Merged
merged 4 commits into from
Mar 18, 2021

Conversation

Hritik14
Copy link
Collaborator

The earlier location http://127.0.0.1:8000/api/docs was a 404. As the
urls.py suggests, the README now reflects http://127.0.0.1:8000/api/schema/swagger-ui/

After the first install, activating the venv is done prior to installing
pytest, thus the system default pytest is being called. It is either
required to reactive venv or run pytest via python -m.

Signed-off-by: Hritik Vijay hritikxx8@gmail.com

The earlier location http://127.0.0.1:8000/api/docs was a 404. As the
urls.py suggests, the README now reflects http://127.0.0.1:8000/api/schema/swagger-ui/

After the first install, activating the venv is done prior to installing
pytest, thus the system default pytest is being called. It is either
required to reactive venv or run pytest via python -m.

Signed-off-by: Hritik Vijay <hritikxx8@gmail.com>
@@ -206,7 +206,7 @@ Run Tests
Use these commands to run code style checks and the test suite::

black -l 100 --check .
DJANGO_DEV=1 pytest
DJANGO_DEV=1 python -m pytest
Copy link
Collaborator

@pombredanne pombredanne Mar 15, 2021

Choose a reason for hiding this comment

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

If you are in an activated virtualenv, pytest should work. Did you experience an issue here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes. As mentioned here, it fails on the first run. I've generated an asciinema to reproduce the issue as well.

@sbs2001
Copy link
Collaborator

sbs2001 commented Mar 16, 2021

The pytest bug is weird. @Hritik14 could you try updating the venv pytest to latest and try running python -m pytest . Just to determine if issue is caused due to system pytest invocation or just some version incompability.

@Hritik14
Copy link
Collaborator Author

Hritik14 commented Mar 17, 2021

@sbs2001 I think you're right. It is failing because the tests are only compatible with pytest version shipped with the project requirements.
Here is an asciinema reproducing my results:
asciicast

As you can see, after first installation, which pytest points to the system pytest.
I have pytest 6.2.2 installed and the project requires pytest==5.3.2.
So, it is both due to system pytest invocation and version incompatibility.

Maybe we can look into this in #348.

In any way, for now, I think the pytest invocation should always be the one shipped with the project itself so as to not just break due to any system update.

@sbs2001
Copy link
Collaborator

sbs2001 commented Mar 17, 2021

@Hritik14 thanks for confirming. Do you mind repasting your comment at #348, that'll be handy.

We'll use python -m pytest as you suggest. It probably doesn't matter much, but I want this to be consistent with the CI. You'll need to update to https://github.com/nexB/vulnerablecode/blob/ff08fd6044110bbd0b7b57010e3622b2fb25c4b6/.travis.yml#L21 and https://github.com/nexB/vulnerablecode/blob/ff08fd6044110bbd0b7b57010e3622b2fb25c4b6/.github/workflows/main.yml#L41

As for the api doc related changes. That's a good check. IMHO the docs should be at http://127.0.0.1:8000/api/docs, the urls.py need some changes.

Here is the entire conversation: aboutcode-org#379

Signed-off-by: Hritik Vijay <hritikxx8@gmail.com>
As mentioned in the previous version of README.rst, the docs should be
at /api/docs.

Signed-off-by: Hritik Vijay <hritikxx8@gmail.com>
@Hritik14
Copy link
Collaborator Author

@sbs2001 Your wish is my command. :)

Copy link
Collaborator

@sbs2001 sbs2001 left a comment

Choose a reason for hiding this comment

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

@Hritik14 thanks for the PR, I'm merging this

@sbs2001 sbs2001 merged commit 80c65a2 into aboutcode-org:main Mar 18, 2021
Pushpit07 pushed a commit to Pushpit07/vulnerablecode that referenced this pull request Mar 18, 2021
Here is the entire conversation: aboutcode-org#379

Signed-off-by: Hritik Vijay <hritikxx8@gmail.com>
Signed-off-by: Pushpit <pushpit07@gmail.com>
Pushpit07 pushed a commit to Pushpit07/vulnerablecode that referenced this pull request Mar 19, 2021
Here is the entire conversation: aboutcode-org#379

Signed-off-by: Hritik Vijay <hritikxx8@gmail.com>
Signed-off-by: Pushpit <pushpit07@gmail.com>
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.

3 participants