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

Integration of pytest #2550

Closed
wants to merge 11 commits into from
Closed

Conversation

polybassa
Copy link
Contributor

This is just a PoC. A *.uts file will be transformed to a pytest like file. The exported file usually needs manual modifications, but this exporter can be a first step towards pytest

@codecov
Copy link

codecov bot commented Mar 26, 2020

Codecov Report

Merging #2550 (0dc7496) into master (2f07f21) will increase coverage by 2.91%.
The diff coverage is n/a.

❗ Current head 0dc7496 differs from pull request most recent head 8e24fdf. Consider uploading reports for the commit 8e24fdf to get more accurate results

@@            Coverage Diff             @@
##           master    #2550      +/-   ##
==========================================
+ Coverage   84.57%   87.49%   +2.91%     
==========================================
  Files         296      267      -29     
  Lines       62137    55824    -6313     
==========================================
- Hits        52555    48845    -3710     
+ Misses       9582     6979    -2603     
Impacted Files Coverage Δ
scapy/contrib/automotive/gm/gmlan_logging.py 73.33% <0.00%> (-26.67%) ⬇️
scapy/layers/l2tp.py 88.88% <0.00%> (-11.12%) ⬇️
scapy/themes.py 80.98% <0.00%> (-10.19%) ⬇️
scapy/modules/p0f.py 64.72% <0.00%> (-9.93%) ⬇️
scapy/contrib/automotive/scanner/test_case.py 76.00% <0.00%> (-8.89%) ⬇️
scapy/layers/tls/crypto/cipher_aead.py 81.30% <0.00%> (-6.53%) ⬇️
scapy/layers/tls/crypto/groups.py 87.14% <0.00%> (-5.82%) ⬇️
scapy/autorun.py 78.23% <0.00%> (-5.21%) ⬇️
scapy/contrib/geneve.py 92.10% <0.00%> (-5.20%) ⬇️
scapy/layers/tls/session.py 83.78% <0.00%> (-4.10%) ⬇️
... and 229 more

@gpotter2
Copy link
Member

Please don't merge this as is.
This is a great script but I'd like to see how well it works (a fully converted file) before doing anything.

I'm still unsure whether it's best to move to pytest because of how massive the work will probably end up being..

@polybassa
Copy link
Contributor Author

polybassa commented Mar 26, 2020 via email

scapy/tools/UTscapy.py Outdated Show resolved Hide resolved
@polybassa polybassa closed this Mar 26, 2020
@polybassa polybassa reopened this Mar 26, 2020
@polybassa polybassa marked this pull request as draft April 28, 2020 08:09
@polybassa
Copy link
Contributor Author

I've added pytest to tox (just for PoC, not added everywhere). What about supporting pytest and UTscapy in parallel?

@polybassa
Copy link
Contributor Author

@gpotter2 @guedou @p-l-
I would like to start a discussion. I've made a small PoC to enable pytest for the unit tests.
I respect the individual configurations by parsing for the argument "-K"
A simple dump function in UTscapy allows to transfer a *.uts file to a pytest file. Manual fixing is required here, nevertheless this script should save some time if one wants to port tests.
I've tested codecov if the combination of tests work corretly, if pytest and uts files for the same unit differ.

Would it make sense to open the unit test system to allow contributors to write tests in *.uts or pytest?

@guedou
Copy link
Member

guedou commented Aug 6, 2020

This is a really cool PR. It its definitely worth finishing it and merging. If @gpotter2 and @p-l- are OK, I think that we should move to pytest and leave UTScapy behind us. After merging this PR, the next step will be to mandate that new unit tests are written with pytest while we migrate all tests.

Did you try concurrent testing? I will be interested to know if moving to pytest will also make the testing time shorter.

@polybassa
Copy link
Contributor Author

Thanks for your feedback. I will continue my work on this PR. I will probably start to port regression.uts to pytest, to see if this concept holds.
I didn't tested concurrent execution, yet. Also my tests on CAN will probably fail if they were executed concurrently. But I will test this, after I ported some tests from regression.uts.

@gpotter2
Copy link
Member

gpotter2 commented Aug 7, 2020

I agree with @guedou that this is very cool, and it would be great to be able to migrate to pytest, but we must make sure everything works before doing that. Thé UTscapy file is filled with tons of extra code that would need to be integrated somehow to our test process.

@polybassa
Copy link
Contributor Author

I totally agree, the UTscapy file is pretty much filled with stuff. I did a rough analysis of the current functionality.. in my opinion, some function might not be useful, some functions will be available in pytest. Most important is the tag system, which I could "somehow" port to pytest.
But I will continue on that PoC, than we can see, where leads to.

@guedou
Copy link
Member

guedou commented Aug 7, 2020

Before converting moving to pytest, I think that it will make sense to split regression.uts into smaller files that respect scapy/ naming convention i.e. test/config.uts test/layers/inet6.uts test/packet.uts ...

First, it will make your life easier when adding or looking for a unit test, then we will be able to convert to pytest file by file with several simple PR instead of a huge one.

@p-l-
Copy link
Member

p-l- commented Aug 7, 2020

That is really great. Thanks a lot for this work.

I agree with @guedou that we may want so split regression.uts.

@polybassa polybassa marked this pull request as ready for review August 12, 2020 08:53
@polybassa polybassa changed the title PoC: Pytest export for *.uts files Integration of pytest Aug 12, 2020
@polybassa
Copy link
Contributor Author

Hi, I've updated this PR and would like to open it for your review.

I made some first ports to pytest. I used the Co-Author tag to preserve the original authors (git blame filename --porcelain | grep "^author " | sort | uniq was used to identify the original authors).

I would suggest the enforcement of flake8 for unit tests. The possibility of pytest would help us to have much cleaner unit tests, in my opinion.

@polybassa
Copy link
Contributor Author

And another question. Should there be a copyright notice on unit tests?

@polybassa
Copy link
Contributor Author

Thanks for your answer. I can understand all your points. In terms of performance you are probably right. The main advantages I see in pytest compared to UTScapy are:

  • Tests are python scripts, which means,
    • blank lines and indentations are supported
    • typing can be applied
    • flake8 can be used on tests
    • codespell can be used on tests
  • Most IDEs have support of pytest, which allows for extensive refactoring features

In my opinion, these additions to the test system will lead us to cleaner and more maintainable tests in the future.

Furthermore, pytest comes with a whole bunch of extensions.

A main problem, that you also mentioned, is the transition from UTScapy to pytest. If there is no way for you to have both systems coexisting for a certain period, for example one release cycle, I totally agree, that a huge effort is required to make a fast switch. Which is almost not affordable for a open source project.

To summarize my opinions, if coexistence of two test systems for a certain time is a no go, we can stop here with pytest.
If not, I would be happy if we could make a plan for a smooth transition over a fixed period, for example the next release cycle.

polybassa and others added 11 commits October 13, 2022 19:28
A small utility in UTscapy should allow a fast porting of existing *.uts files
Co-Authored-By: Gabriel Potter <gabriel@potter.fr>
Co-Authored-By: Pierre LALET <pierre.lalet@cea.fr>
Co-Authored-By: Guillaume Valadon <guillaume@valadon.net>
Co-Authored-By: Gabriel Potter <gabriel@potter.fr>
Co-Authored-By: Gabriel Potter <gabriel@potter.fr>
Co-Authored-By: Pierre LALET <pierre.lalet@cea.fr>
Co-Authored-By: sebastien mainand <sebastien.mainand@ssi.gouv.fr>
Co-Authored-By: Gabriel Potter <gabriel@potter.fr>
Co-Authored-By: Andreas Korb <Andreas.D.Korb@gmail.com>
…nsocket_python_can.py

Co-Authored-By: Gabriel Potter <gabriel@potter.fr>
Co-Authored-By: Andreas Korb <Andreas.D.Korb@gmail.com>
Co-Authored-By: Guillaume Valadon <guillaume@valadon.net>
Co-authored-by: Phil <phil@secdev.org>
Co-authored-by: gpotter2 <gabriel@potter.fr>
Co-authored-by: Michael Farrell <micolous+git@gmail.com>
Co-authored-by: Pierre Lalet <pierre@droids-corp.org>
Co-Authored-By: Guillaume Valadon <guillaume@valadon.net>
Co-authored-by: Pierre Lalet <pierre@droids-corp.org>
…UTscapy"

This reverts commit c28b9affa63ffbdc8f0bc2eb8770faa8a1455fb5.
@polybassa
Copy link
Contributor Author

closed, because its broken

@polybassa polybassa closed this Oct 13, 2022
@gpotter2
Copy link
Member

Honnestly I think it's a decent idea, and maybe one day we'll be happy to have it as a basis.
It's just too much work for now, but who knows 😛

@evverx
Copy link
Contributor

evverx commented Apr 4, 2023

The main advantages I see in pytest compared to UTScapy

I'd add that pytest should make it much easier to catch resource warnings like #3962 (comment) as well as other warnings produced by python3 -Xdev -Werror. It took me about a week to figure out where that particular warning went :-)

UTScapy and pytest should not coexist

I think it would be easier to switch to pytest if it was possible to convert the tests one by one.

@polybassa
Copy link
Contributor Author

Hello together, I would like to come back to this issue. I'm wondering, if we could start supporting pytest, in order to have to opportunity to start a transition from UTScapy to pytest.

@polybassa
Copy link
Contributor Author

I've made a few tests, apparently chatGPT is really useful to translate unit tests from UTScapy to pytest. This would allow a transition from UTScapy to pytest with a fraction of the effort, compared to doing everything manually.

@guedou
Copy link
Member

guedou commented Jun 13, 2023 via email

@KhazAkar
Copy link
Contributor

KhazAkar commented Oct 3, 2023

For me @guedou your idea makes sense. I will someday sit, reading tests/ folder contents, search for UTScapy specific behavior and start porting to pytest, folder by folder. Will take time, but I will step up to it :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants