Skip to content

#2158 - Add Hinty for plist.py - Adds mypy from Issue #2158 #2358

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

Merged
merged 6 commits into from
Dec 9, 2019

Conversation

akshaycbor
Copy link
Contributor

@akshaycbor akshaycbor commented Dec 2, 2019

Not all functions were covered due to mypy issues

Cannot determine type after isInstance check - PacketList.init()
python/mypy#6846

Issue when a None method is recasted using an inner def - PacketList.conversations()
python/mypy#2608

part of #2158

@guedou
Copy link
Member

guedou commented Dec 2, 2019

Thanks for your PR. See my comment.

@codecov
Copy link

codecov bot commented Dec 2, 2019

Codecov Report

Merging #2358 into master will increase coverage by 0.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #2358      +/-   ##
==========================================
+ Coverage    87.8%   87.82%   +0.01%     
==========================================
  Files         243      243              
  Lines       50735    50749      +14     
==========================================
+ Hits        44549    44570      +21     
+ Misses       6186     6179       -7
Impacted Files Coverage Δ
scapy/plist.py 86.97% <100%> (+0.1%) ⬆️
scapy/extlib.py 86.11% <100%> (+0.81%) ⬆️
scapy/contrib/cdp.py 90.13% <0%> (-0.78%) ⬇️
scapy/layers/inet6.py 81.75% <0%> (-0.27%) ⬇️
scapy/layers/ntp.py 91.77% <0%> (ø) ⬆️
scapy/layers/inet.py 73.1% <0%> (+0.16%) ⬆️
scapy/asn1/ber.py 83.69% <0%> (+0.27%) ⬆️
scapy/layers/tls/record.py 90.46% <0%> (+0.86%) ⬆️
scapy/layers/tls/record_sslv2.py 87.7% <0%> (+1.67%) ⬆️
scapy/autorun.py 82.64% <0%> (+4.13%) ⬆️

@akshaycbor
Copy link
Contributor Author

4 build jobs are failing with the following error
ERROR: InvocationError for command /usr/bin/sudo -E .tox/py27-linux_root/bin/python -m coverage run -a -m scapy.tools.UTscapy -c ./test/configs/linux.utsc -K random_weird_py3 -K tshark (exited with code 1)

Not sure what is causing that

Copy link
Member

@guedou guedou left a comment

Choose a reason for hiding this comment

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

I did a complete review and commented on the types that can be stricter.

@akshaycbor
Copy link
Contributor Author

One test in regression.uts is failing because of the addition to extlib.py made in this commit.

>>> with patch.dict("sys.modules", matplotlib=matplotlib):
...     from scapy.extlib import MATPLOTLIB, MATPLOTLIB_INLINED, MATPLOTLIB_DEFAULT_PLOT_KARGS
...     assert MATPLOTLIB == 1
...     assert MATPLOTLIB_INLINED == 1
...     assert "marker" in MATPLOTLIB_DEFAULT_PLOT_KARGS

Seems like the extlib import is failing even when it's not supposed to. But I cannot figure out what's causing that.
This is my first commit on scapy so I'm not aware of what configuration and/or test changes I might be missing here

@guedou
Copy link
Member

guedou commented Dec 4, 2019

This fixes the issue:

$ git diff
diff --git a/test/regression.uts b/test/regression.uts
index 9ee9eb95..87a81efd 100644
--- a/test/regression.uts
+++ b/test/regression.uts
@@ -680,18 +680,19 @@ from mock import MagicMock, patch
 bck_scapy_ext_lib = sys.modules.get("scapy.extlib", None)
 del(sys.modules["scapy.extlib"])
 
-matplotlib = MagicMock()
-matplotlib.get_backend.return_value = "inline"
-matplotlib.pyplot = MagicMock()
-matplotlib.pyplot.plt = None
-with patch.dict("sys.modules", matplotlib=matplotlib):
-    from scapy.extlib import MATPLOTLIB, MATPLOTLIB_INLINED, MATPLOTLIB_DEFAULT_PLOT_KARGS
+mock_matplotlib = MagicMock()
+mock_matplotlib.get_backend.return_value = "inline"
+mock_matplotlib.pyplot = MagicMock()
+mock_matplotlib.pyplot.plt = None
+with patch.dict("sys.modules", **{ "matplotlib": mock_matplotlib, "matplotlib.lines": mock_matplotlib}):
+    from scapy.extlib import MATPLOTLIB, MATPLOTLIB_INLINED, MATPLOTLIB_DEFAULT_PLOT_KARGS, Line2D
+    print(MATPLOTLIB,MATPLOTLIB_INLINED,MATPLOTLIB_DEFAULT_PLOT_KARGS, Line2D)
     assert MATPLOTLIB == 1
     assert MATPLOTLIB_INLINED == 1
     assert "marker" in MATPLOTLIB_DEFAULT_PLOT_KARGS
 
-matplotlib.get_backend.return_value = "ko"
-with patch.dict("sys.modules", matplotlib=matplotlib):
+mock_matplotlib.get_backend.return_value = "ko"
+with patch.dict("sys.modules", **{ "matplotlib": mock_matplotlib, "matplotlib.lines": mock_matplotlib}):
     from scapy.extlib import MATPLOTLIB, MATPLOTLIB_INLINED, MATPLOTLIB_DEFAULT_PLOT_KARGS
     assert MATPLOTLIB == 1
     assert MATPLOTLIB_INLINED == 0

gpotter2
gpotter2 previously approved these changes Dec 6, 2019
@gpotter2 gpotter2 added the Hinty label Dec 6, 2019
Copy link
Member

@guedou guedou left a comment

Choose a reason for hiding this comment

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

I made one final comment.

scapy/plist.py Outdated
@@ -13,16 +13,20 @@
import os
from collections import defaultdict


Copy link
Member

Choose a reason for hiding this comment

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

This changes is not needed. Can you remove it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've removed this change

@gpotter2
Copy link
Member

gpotter2 commented Dec 6, 2019

Don't worry about AppVeyor.
Chocolatey has been broken for the past week...
https://status.chocolatey.org/

@guedou guedou merged commit b49af41 into secdev:master Dec 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants