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

Use GitHub Actions for testing #97

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

Conversation

doismellburning
Copy link
Member

Free Travis CI is no more

@doismellburning
Copy link
Member Author

This should be a draft PR really, and the scope blew up - GitHub Actions was fiddly to get working with Python 2.7, so I started looking at a Python 3 upgrade (because there was the kernel of dual support there anyway)

@doismellburning
Copy link
Member Author

Ok I thought I could get away with a cheeky 2->3 port along the way, but this does too much complex bit-twiddling for that to Just Work easily

I see two plausible approaches here:

  1. Go through the pain of sorting a solid Python 2.7 GitHub Actions setup for good testing, then maybe add some better linters or other testing, then upgrade to 3. This is probably the most reliable way to not accidentally break anything
  2. Continue with this 3 port as-is, introducing some modern lint/testing tech to hopefully uncover the porting issues

1 is fiddlier - Python 2.7 is old, tooling and infra support is limited. BUT this worked on 2.7, so slowly incrementing from there feels like the best way to not introduce bugs. Option 2 feels far more likely to introduce bugs as part of the porting process, though it would be “easier”.

I think the right way forward is to attempt 1, falling back to 2 only if it seems too impractical

@iblislin
Copy link

I will vote for option 1. since there might be some migration issues on deps (especially Twisted, I think), we can work on the 2to3 first, then deps.

@iblislin
Copy link

iblislin commented May 16, 2024

I googled about py27 with Actions.
This snippet might help: actions/setup-python#672 (comment)

@doismellburning
Copy link
Member Author

Thanks! #98 means we now have Python 2.7 automated tests - time to throw a linter or so at it and make it a better Python 2 project...!

@doismellburning
Copy link
Member Author

Unfortunately flake8 on Python 2.7 isn't identifying any significant issues (beyond the port one you identified) - I was hoping it might complain about a bunch of things where fixing them would make the 3 migration easier...!

@doismellburning
Copy link
Member Author

(I'll keep this open for a bit until I file a porting PR!)

@iblislin
Copy link

iblislin commented May 22, 2024

I tried the 2to3.py, but seems that bytes & str issues are not discovered by it. only few patches are shown:

diff --git a/einstein/hookserver.py b/einstein/hookserver.py
index 93d3c44..e1f255f 100644
--- a/einstein/hookserver.py
+++ b/einstein/hookserver.py
@@ -21,7 +21,7 @@ class HookServer(object):

     @app.route("/")
     def list(self, request):
-        return str(self.latest_observations.values())
+        return str(list(self.latest_observations.values()))
diff --git a/einstein/intellivue/test_asn_length_field.py b/einstein/intellivue/test_asn_length_field.py
index 305bf26..66cdccc 100644
--- a/einstein/intellivue/test_asn_length_field.py
+++ b/einstein/intellivue/test_asn_length_field.py
@@ -1,4 +1,4 @@
-from association import AssocReqUserData, MDSEUserInfoStd
+from .association import AssocReqUserData, MDSEUserInfoStd


 def test_trivial_construction():
diff --git a/einstein/intellivue/test_association.py b/einstein/intellivue/test_association.py
index 2817ca2..bde7bbc 100644
--- a/einstein/intellivue/test_association.py
+++ b/einstein/intellivue/test_association.py
@@ -1,4 +1,4 @@
-from association import *
+from .association import *

 def test_association():
     pass

@iblislin
Copy link

iblislin commented May 22, 2024

ah, the last two ☝️ are included in this PR already.

@iblislin
Copy link

Fix test_empty_length (https://github.com/somno/einstein/actions/runs/9181914312/job/25249720529#step:6:58)

diff --git a/einstein/intellivue/association.py b/einstein/intellivue/association.py
index a349207..b6cb4cb 100644
--- a/einstein/intellivue/association.py
+++ b/einstein/intellivue/association.py
@@ -106,7 +106,7 @@ class ASNLengthField(FieldLenField):  # PIPG-68
         return s + b
 
     def getfield(self, pkt, s):
-        v = struct.unpack("B", s[0])[0]
+        v = struct.unpack("B", s[:1])[0]
         if v & (1<<7):
             v &= ~(1<<7)
             raise NotImplementedError

@iblislin
Copy link

these patches fix all the rest of testing errors.

diff --git a/einstein/intellivue/association.py b/einstein/intellivue/association.py
index a349207..00bb3f7 100644
--- a/einstein/intellivue/association.py
+++ b/einstein/intellivue/association.py
@@ -23,11 +23,11 @@ class LIField(Field):
         if val < 255:
             b = struct.pack("B", val)
         else:
-            b = '\xff' + struct.pack("!H", val)
+            b = b'\xff' + struct.pack("!H", val)
         return s + b
 
     def getfield(self, pkt, s):
-        if s[0] == '\xff':
+        if s[0] == 255:  # where 255 is just b'\xff'
             return s[3:], self.m2i(pkt, struct.unpack("!H", s[1:3])[0])
         else:
             return s[1:], self.m2i(pkt, struct.unpack("B", s[:1])[0])
diff --git a/einstein/test_lifield.py b/einstein/test_lifield.py
index 4bc965b..a517c28 100644
--- a/einstein/test_lifield.py
+++ b/einstein/test_lifield.py
@@ -20,36 +20,36 @@ def test_zero_encoded():
     assert p.length == 0
     data = raw(p)
     assert len(data) == 1
-    assert data[0] == '\x00'
+    assert data[:1] == b'\x00'
 
 
 def test_small_length():
     count = 4
-    p = LIPacket() / ("A" * count)
+    p = LIPacket() / (b"A" * count)
     p = LIPacket(p.build())
     assert p.length == count
 
 
 def test_small_encoded():
     count = 4
-    p = LIPacket() / ("A" * count)
+    p = LIPacket() / (b"A" * count)
     p = LIPacket(p.build())
     assert p.length == count
     data = raw(p)
     assert len(data) == 1 + count
-    assert data[0] == b"\x04"  # count
+    assert data[:1] == b"\x04"  # count
 
 
 def test_large_length():
     count = 300
-    p = LIPacket() / ("A" * count)
+    p = LIPacket() / (b"A" * count)
     p = LIPacket(p.build())
     assert p.length == count
 
 
 def test_large_encoded():
     count = 300
-    p = LIPacket() / ("A" * count)
+    p = LIPacket() / (b"A" * count)
     p = LIPacket(p.build())
     assert p.length == count
     data = raw(p)

@iblislin
Copy link

iblislin commented Jun 3, 2024

fine to go?

@iblislin
Copy link

bump

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