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

New sign tra #125

Merged
merged 8 commits into from
Aug 25, 2023
Merged

New sign tra #125

merged 8 commits into from
Aug 25, 2023

Conversation

HanslettTheDev
Copy link
Collaborator

@HanslettTheDev HanslettTheDev commented Jul 11, 2023

Summary

This pull request aims to split the sign tra function based on this comment from @reingart.
Unit tests will also be added for all the functions to ensure they are properly tested and maintain the standards of the previous general function

Checklist

  • Classes, Variables, function and methods logic ok
  • Comments written explaining what the code does

…lled each one from the sin tra section

Signed-off-by: HanslettTheDev <HanslettTheDev@gmail.com>
Signed-off-by: HanslettTheDev <HanslettTheDev@gmail.com>
Signed-off-by: HanslettTheDev <HanslettTheDev@gmail.com>
Copy link
Member

@reingart reingart 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 @HanslettTheDev for the contribution!
I made some comments about styling and rewriting some conditions.
Don't forget to add the Unit Tests so this new functions could get coverage.

wsaa.py Outdated Show resolved Hide resolved
wsaa.py Outdated Show resolved Hide resolved
wsaa.py Outdated Show resolved Hide resolved
Signed-off-by: HanslettTheDev <HanslettTheDev@gmail.com>
Copy link
Member

@reingart reingart left a comment

Choose a reason for hiding this comment

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

Hi @HanslettTheDev , the tests doesn't have any new UT (all changes are just whitespaces)

At least a test_sign_tra_openssl function should be added, that would work in any python / cryptography version.

For the old function version, you could just add a manual tests run evidence (running the python 2 virtualenv and attaching tests results)

TIP: use pytest coverage report:

pytest --cov-report term-missing --cov=pyafipws.wsaa tests/test_wsaa.py

Then you could see the report for wsaa.py file and confirm that the lines are being covered (exercised by the test):

---------- coverage: platform linux, python 3.10.6-final-0 -----------
Name      Stmts   Miss  Cover   Missing
---------------------------------------
wsaa.py     352     89    75%   58-65, 130, 177-199, 203-215, 227, 230-231, 274-276, 294, 323-325, 353-355, 380, 392, 406, 438, 445, 468, 474, 480-481, 485, 491-500, 513-533, 536-540, 552-561, 580-584, 613-614, 627-629, 635, 645
---------------------------------------
TOTAL       352     89    75%

@HanslettTheDev
Copy link
Collaborator Author

The unit testing functions are with me locally. I was working on a bunch of things so I will push all the commits today

Signed-off-by: HanslettTheDev <HanslettTheDev@gmail.com>
@HanslettTheDev
Copy link
Collaborator Author

Manual test evidence of Pytest Test coverage on Python 2.7
image

@HanslettTheDev HanslettTheDev requested a review from reingart August 6, 2023 18:10
wsaa.py Outdated Show resolved Hide resolved
…ography version > 100

Signed-off-by: HanslettTheDev <HanslettTheDev@gmail.com>
wsaa.py Outdated Show resolved Hide resolved
wsaa.py Outdated Show resolved Hide resolved
Signed-off-by: HanslettTheDev <HanslettTheDev@gmail.com>
Signed-off-by: HanslettTheDev <HanslettTheDev@gmail.com>
@HanslettTheDev
Copy link
Collaborator Author

Coverage Report for both Python 2.7 and 3+

htmlcov-python3+.zip
htmlcov-python27.zip

@reingart
Copy link
Member

Thanks for the coverage! the python2 zip seems to miss the wsaa.py html, could you re-upload or generate a new report?

@HanslettTheDev
Copy link
Collaborator Author

Reuploaded the test coverage report for python2.7
htmlcov-python27.zip

@reingart reingart merged commit 35387c3 into PyAr:main Aug 25, 2023
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