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

test: add pytest and build details unit tests #969

Merged
merged 1 commit into from
Feb 21, 2025
Merged

Conversation

murilx
Copy link
Contributor

@murilx murilx commented Feb 19, 2025

  • Added pytest
  • Added pytest-django (not being used for now but expected to be added in the future)
  • Run tests before pushing
  • Updated the existing unit tests to use pytest instead of unittest
  • Added utilities to perform django 'external' unit tests
  • Added tests for BuildDetailsView as an example for the other django
    tests to be performed

Part of #943
Closes #995

How to test

  • Make sure you are running the backend with poetry run python3 manage.py runserver
  • Run the unit tests poetry run pytest

@murilx murilx self-assigned this Feb 20, 2025
@murilx murilx force-pushed the chore/add-pytest branch 2 times, most recently from c02c108 to 46403d5 Compare February 20, 2025 19:02
@murilx murilx marked this pull request as ready for review February 20, 2025 19:04
@murilx murilx force-pushed the chore/add-pytest branch 6 times, most recently from 47ea1ae to 6737899 Compare February 21, 2025 12:38
@murilx murilx force-pushed the chore/add-pytest branch 2 times, most recently from ede620a to ca8e54b Compare February 21, 2025 12:54
Copy link
Collaborator

@MarceloRobert MarceloRobert left a comment

Choose a reason for hiding this comment

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

It seems that it is trying to get '/' and not finding it, is that expected?

Also, if I run pytest with the environment variables set but the backend not running it doesn't exit, but I think that it should exit.

It is also showing 40% for the buildDetails test and a deprecation warning for a certain config, is that expected?

@murilx
Copy link
Contributor Author

murilx commented Feb 21, 2025

@MarceloRobert

It seems that it is trying to get '/' and not finding it, is that expected?

Yes, this is intended. Maybe there's another way but my idea was to check if the server is online and for that I can hit a non-existent endpoint since it doesn't result in a connection error or timeout (the two things I'm considering as server offline). I also chose this because it's probably one of the faster ways of checking this, instead of having to wait for the server to process and return data I'm not interested in. I tried sending ICMP packets (pinging) the server, but from my experience it'll always result in a positive result, even if the server isn't running because it's on localhost (you can try running ping localhost)

Also, if I run pytest with the environment variables set but the backend not running it doesn't exit, but I think that it should exit.

This is a behavior I also saw and only happens if not all tests pass, so if there's any skips it'll continue running. I'll check if there's any flag I can pass pytest to change this behavior

It is also showing 40% for the buildDetails test and a deprecation warning for a certain config, is that expected?

40% I believe is the progress of the test suite. So if there's 5 tests in total, the first one would represent 20% of the test suite completed, an so on. I might be wrong though, I can check this further. The deprecation warning is related to another part of the source code, nothing that I added or related to what I added. For that reason I thought this should be solved in another PR. What do you think?

@murilx murilx force-pushed the chore/add-pytest branch 2 times, most recently from 929425b to 4b8b03d Compare February 21, 2025 13:53
@MarceloRobert
Copy link
Collaborator

@murilx about the '/' ping, that's ok then, if there were an easy way to ping a specific port would be ideal but I think that that is good enough, just add this to the readme so other people don't get confused as I did

For the 40%, you're right, it might just be the total progress of the tests, and the deprecation warning seems to be from pydantic so I guess we can leave it to another future issue.

@@ -0,0 +1,16 @@
import requests
Copy link
Collaborator

Choose a reason for hiding this comment

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

rename this file to something like health-check or status-check

@murilx
Copy link
Contributor Author

murilx commented Feb 21, 2025

@MarceloRobert I added a note to the README, please check it out. About pytest not existing after finishing the tests, it's seems to be caused by pytest-django. I didn't find the real reason for it but pytest-django seems to be needed (tried to remove it and it resulted in some errors). I'll leave it as is for now to focus in implementing the separated classes we discussed on Slack, but I'll try to find a solution later. About the 40%, it's, in fact, the progress for all tests

Captura de tela de 2025-02-21 11-00-04


def ping(host: str) -> bool:
"""
Checks if a host is online using the Linux 'ping' command.
Copy link
Collaborator

@WilsonNet WilsonNet Feb 21, 2025

Choose a reason for hiding this comment

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

I think this is outdated since you are using requests.get now which I imagine is an http request

@WilsonNet
Copy link
Collaborator

WilsonNet commented Feb 21, 2025

@MarceloRobert

Yes, this is intended. Maybe there's another way but my idea was to check if the server is online and for that I can hit a non-existent endpoint since it doesn't result in a connection error or timeout (the two things I'm considering as server offline). I also chose this because it's probably one of the faster ways of checking this, instead of having to wait for the server to process and return data I'm not interested in. I tried sending ICMP packets (pinging) the server, but from my experience it'll always result in a positive result, even if the server isn't running because it's on localhost (you can try running ping localhost)

You can create an helthCheckView that simply returns an { status: "OK" } feels a little less hacky and it allow us to scale for other health checks

@murilx murilx force-pushed the chore/add-pytest branch 2 times, most recently from f903361 to 3354618 Compare February 21, 2025 14:59
@murilx
Copy link
Contributor Author

murilx commented Feb 21, 2025

@WilsonNet initially tried a solution with sockets, since I didn't thought a endpoint for this should be necessary (at least at this moment). If you still think it's best we add this endpoint and do a request to it to verify if the server is online, tell me so I can update the function again

Copy link
Collaborator

@WilsonNet WilsonNet left a comment

Choose a reason for hiding this comment

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

working fine here

Copy link
Collaborator

@Francisco2002 Francisco2002 left a comment

Choose a reason for hiding this comment

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

Nice job!!

- Added pytest
- Added pytest-django (not being used for now)
- Run tests before pushing
- Updated the existing unit tests to use pytest instead of unittest
- Added utilities to perform django 'external' unit tests
- Added tests for BuildDetailsView as an example for the other django
  tests to be performed

Part of #943
Closes #995
@murilx murilx merged commit 91a10e6 into main Feb 21, 2025
5 checks passed
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.

Unit tests initial Setup
4 participants