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

feature: unittests #57

Merged
merged 9 commits into from
Jun 18, 2022
Merged

feature: unittests #57

merged 9 commits into from
Jun 18, 2022

Conversation

RYNEQ
Copy link
Collaborator

@RYNEQ RYNEQ commented Jun 1, 2022

This is a tryout to add UnitTests to tracevis
we can now test by python -m unittest discover test -b
It is not complete yet so PR should wait until most of important tests be complete

@xhdix xhdix requested a review from bassosimone June 1, 2022 09:17
@xhdix xhdix added enhancement New feature or request priority/high labels Jun 1, 2022
@xhdix xhdix added this to In progress in Wiki Censorship via automation Jun 1, 2022
bassosimone
bassosimone previously approved these changes Jun 3, 2022
Copy link
Contributor

@bassosimone bassosimone left a comment

Choose a reason for hiding this comment

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

Thanks for working on unit tests to ensure the CLI API does not break unexpectedly!

🙏 🥳 🐳 👍

@bassosimone
Copy link
Contributor

It is not complete yet so PR should wait until most of important tests be complete

Yeah. It's already a good start to have these unit tests and we can add more. Perhaps, it would be cool to add a GitHub action for running tests automatically at every commit and PR before merging, if you have time for that!

@RYNEQ
Copy link
Collaborator Author

RYNEQ commented Jun 5, 2022

Yeah. It's already a good start to have these unit tests and we can add more. Perhaps, it would be cool to add a GitHub action for running tests automatically at every commit and PR before merging, if you have time for that!

Thanks for the review and comments
Yes we should add more tests specially for core functionalities but it can be progressive and every new test can be added to previous ones in the future
So it is a good idea to start having actions for the ones already exist

I also think having a code coverage test in actions can help to show how much of the codebase is under test

- `test_config_file` unittest fixed for missing keys in config file
Now it tests against existing keys in config file and ignores the rest
(the rest of keys are default and previously are tested in `test_defaults`)

- Added coverage package to make code covarage test possible
@RYNEQ
Copy link
Collaborator Author

RYNEQ commented Jun 5, 2022

  • test_config_file unittest fixed for missing keys in config file
    Now it tests against existing keys in config file and ignores the rest (the rest of keys are defaults which previously tested in test_defaults)

  • Added coverage package to make code covarage test possible
    adding coverage before test command makes test report and coverage report shows it

Name                          Stmts   Miss  Cover
-------------------------------------------------
test/test_operationality.py      79      0   100%
tracevis.py                     215    140    35%
utils/__init__.py                 0      0   100%
utils/convert_packetlist.py      25     22    12%
utils/csv.py                     82     73    11%
utils/dns.py                     16     11    31%
utils/ephemeral_port.py          23     18    22%
utils/geolocate.py               69     56    19%
utils/packet_input.py           201    160    20%
utils/ripe_atlas.py              40     34    15%
utils/trace.py                  410    366    11%
utils/traceroute_struct.py       57     49    14%
utils/vis.py                    287    252    12%
-------------------------------------------------
TOTAL                          1504   1181    21%

@xhdix xhdix requested a review from bassosimone June 6, 2022 22:09
Copy link
Contributor

@bassosimone bassosimone left a comment

Choose a reason for hiding this comment

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

🥳 Thanks for the improved tests! Could you also commit a github action that runs tests at every commit and pull request? 🙏

a github action file for running unittest on PR and pushes with action
result badge in README.md
@RYNEQ
Copy link
Collaborator Author

RYNEQ commented Jun 9, 2022

partying_face Thanks for the improved tests! Could you also commit a github action that runs tests at every commit and pull request? pray

Hi @bassosimone , I added the unittest action file.
for code coverage test we can have similar action file wich only can show pass or fail result. so to show coverage percent in README.md we need to generate a custom badge. we have three options:

  • Use cloud test providers (e.g. codecov) which will need GH_TOKEN to generate badge for us
  • Calculate percentage using simple bash and use badge generator URLs like googleapi or shields.io, which need to include external URL in README.md and update README file on every push (this adds additional change to README which is unrelated to commit subject)
  • Generate badge using python in action file and store its image in somewhere (e.g. .github dir) and use it in README.md

What is your opinion? Which one is the best approach?

@bassosimone
Copy link
Contributor

@RYNEQ At OONI, we generally use codecov or coveralls and link their badge, which, as you say, requires to setup some form of communication between GitHub and one of those services. The other two options that you mention strike me as a bit more work, so perhaps we should try to avoid using them, unless there's some reason not to trust an external coverage service. In such a case, I would just ignore generating the badge and make hack some code to make the GitHub action fail if the coverage decreases by more than, say, 0.5%.

bassosimone
bassosimone previously approved these changes Jun 15, 2022
Copy link
Contributor

@bassosimone bassosimone left a comment

Choose a reason for hiding this comment

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

(Your subsequent changes LGTM! 🐳 )

Copy link
Contributor

@xhdix xhdix left a comment

Choose a reason for hiding this comment

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

Thank you @RYNEQ and @bassosimone! ❤️ 🌺
I'm going to merge this. There will be follow-up PR for adding Coveralls workflow.

@xhdix xhdix merged commit 63bc2df into main Jun 18, 2022
Wiki Censorship automation moved this from In progress to Done Jun 18, 2022
@xhdix xhdix deleted the unittest branch June 18, 2022 18:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request priority/high
Projects
Development

Successfully merging this pull request may close these issues.

None yet

3 participants