Bitmessage/PyBitmessage-2024-11-28
Archived
2
0
Fork 0
Code Issues 168 Pull Requests 53 Actions Packages Projects Releases 5 Wiki Activity

Python3 port for test_networkgroup.py, test_protocol.py #1792

Closed
kdcis wants to merge 1 commits from v0.6-test-networkgroup into v0.6
pull from: v0.6-test-networkgroup
merge into: Bitmessage:v0.6
Conversation 5 Commits 1 Files Changed 3 +104 -71
kdcis commented 2021-07-27 14:29:56 +02:00 (Migrated from github.com)
Copy Link

Fix Imports
Contains changes from #1786 for protocol.py as it needed to run test_networkgroup
Handled bytes to string comparison/concatenation cases
Python 2/3 compatibility

Fix Imports Contains changes from #1786 for protocol.py as it needed to run test_networkgroup Handled bytes to string comparison/concatenation cases Python 2/3 compatibility
PeterSurda (Migrated from github.com) requested changes 2021-07-28 09:47:10 +02:00
src/highlevelcrypto.py
PeterSurda (Migrated from github.com) commented 2021-07-28 09:33:20 +02:00

I'm not sure about this. Need to think about it.

I'm not sure about this. Need to think about it.
src/protocol.py
PeterSurda (Migrated from github.com) commented 2021-07-28 09:33:35 +02:00

I'm also not sure about these imports.

I'm also not sure about these imports.
PeterSurda (Migrated from github.com) commented 2021-07-28 09:38:36 +02:00

This isn't needed, use six.PY2.

This isn't needed, use `six.PY2`.
src/protocol.py
@ -101,3 +101,3 @@
host.split(".")[0], True)
elif host.find(':') == -1:
return '\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\xFF\xFF' + \
return b'\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\xFF\xFF' + \
PeterSurda (Migrated from github.com) commented 2021-07-28 09:40:29 +02:00

Here I would refactor the original code. Create new constants for the raw strings ,and then you can keep the old codepath inside the function. Like this:

if sx.PY2:
    ONION_PREFIX = '\xfd\x87\xd8\x7e\xeb\x43'
else:
    ONION_PREFIX = b'\xfd\x87\xd8\x7e\xeb\x43'
Here I would refactor the original code. Create new constants for the raw strings ,and then you can keep the old codepath inside the function. Like this: ``` if sx.PY2: ONION_PREFIX = '\xfd\x87\xd8\x7e\xeb\x43' else: ONION_PREFIX = b'\xfd\x87\xd8\x7e\xeb\x43' ```
src/protocol.py
@ -164,3 +164,3 @@
if hostStandardFormat == "":
if hostStandardFormat:
# This can happen on Windows systems which are
# not 64-bit compatible so let us drop the IPv6 address.
PeterSurda (Migrated from github.com) commented 2021-07-28 09:41:48 +02:00

Same as with encodeHost: refactor to use a constant and six. The constants are the same as with encodeHost, no need to duplicate.

Same as with `encodeHost`: refactor to use a constant and `six`. The constants are the same as with `encodeHost`, no need to duplicate.
PeterSurda (Migrated from github.com) commented 2021-07-28 09:42:34 +02:00

Same as above, refactor using constants and six.

Same as above, refactor using constants and `six`.
src/protocol.py
@ -284,3 +287,4 @@
def CreatePacket(command, payload=b''):
"""Construct and return a packet"""
payload_length = len(payload)
checksum = hashlib.sha512(payload).digest()[0:4]
PeterSurda (Migrated from github.com) commented 2021-07-28 09:45:19 +02:00

I don't know if this is ok as there are no tests for this. So you need to write a test.

I don't know if this is ok as there are no tests for this. So you need to write a test.
src/tests/common.py
PeterSurda (Migrated from github.com) commented 2021-07-28 09:45:31 +02:00

Again I'm not sure about this.

Again I'm not sure about this.
src/tests/test_networkgroup.py
PeterSurda (Migrated from github.com) commented 2021-07-28 09:45:52 +02:00

You don't need sys, use six instead.

You don't need `sys`, use `six` instead.
PeterSurda (Migrated from github.com) commented 2021-07-28 09:46:21 +02:00

Again not sure, I need to think about it.

Again not sure, I need to think about it.
PeterSurda (Migrated from github.com) commented 2021-07-28 09:46:54 +02:00

Use constants or class variables.

Use constants or class variables.
PeterSurda (Migrated from github.com) commented 2021-07-28 09:47:04 +02:00

same here.

same here.
kdcis (Migrated from github.com) reviewed 2021-07-28 15:20:00 +02:00
src/protocol.py
@ -284,3 +287,4 @@
def CreatePacket(command, payload=b''):
"""Construct and return a packet"""
payload_length = len(payload)
checksum = hashlib.sha512(payload).digest()[0:4]
kdcis (Migrated from github.com) commented 2021-07-28 15:19:59 +02:00

Okay, I'll write one

Okay, I'll write one
g1itch (Migrated from github.com) reviewed 2021-07-28 15:50:55 +02:00
src/highlevelcrypto.py
g1itch (Migrated from github.com) commented 2021-07-28 15:50:55 +02:00

You don't need this, please look into #1788

You don't need this, please look into #1788
g1itch (Migrated from github.com) reviewed 2021-07-28 15:53:44 +02:00
src/highlevelcrypto.py
g1itch (Migrated from github.com) commented 2021-07-28 15:53:43 +02:00

However in tests the imports gonna be like these. See: a8c79c3832e2e3081ebdcb0373f6b4c0057906f7.

However in tests the imports gonna be like these. See: a8c79c3832e2e3081ebdcb0373f6b4c0057906f7.
kdcis (Migrated from github.com) reviewed 2021-07-28 15:57:25 +02:00
src/highlevelcrypto.py
kdcis (Migrated from github.com) commented 2021-07-28 15:57:25 +02:00

Okay, got it

Okay, got it
g1itch (Migrated from github.com) reviewed 2021-07-28 16:03:19 +02:00
src/protocol.py
@ -284,3 +287,4 @@
def CreatePacket(command, payload=b''):
"""Construct and return a packet"""
payload_length = len(payload)
checksum = hashlib.sha512(payload).digest()[0:4]
g1itch (Migrated from github.com) commented 2021-07-28 16:03:18 +02:00

I've already solved this in 7e1f6e8. Yes, at first I wrote more tests.

I've already solved this in 7e1f6e8. Yes, at first I wrote more tests.
g1itch (Migrated from github.com) reviewed 2021-07-28 16:07:09 +02:00
src/protocol.py
@ -101,3 +101,3 @@
host.split(".")[0], True)
elif host.find(':') == -1:
return '\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\xFF\xFF' + \
return b'\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\xFF\xFF' + \
g1itch (Migrated from github.com) commented 2021-07-28 16:07:09 +02:00

python2.7 has no problem with bytes literals

python2.7 has no problem with bytes literals
kdcis (Migrated from github.com) reviewed 2021-07-28 16:39:32 +02:00
src/protocol.py
@ -284,3 +287,4 @@
def CreatePacket(command, payload=b''):
"""Construct and return a packet"""
payload_length = len(payload)
checksum = hashlib.sha512(payload).digest()[0:4]
kdcis (Migrated from github.com) commented 2021-07-28 16:39:32 +02:00

Okay, I'll merge your changes

Okay, I'll merge your changes
PeterSurda (Migrated from github.com) requested changes 2021-08-03 12:14:53 +02:00
src/protocol.py
PeterSurda (Migrated from github.com) commented 2021-08-03 10:26:20 +02:00

maybe
if not hostStandardFormat:
?

check bool(b'') and bool(b'1') in python2 and python3 to make sure it works

maybe `if not hostStandardFormat:` ? check `bool(b'')` and `bool(b'1')` in python2 and python3 to make sure it works
src/protocol.py
PeterSurda (Migrated from github.com) commented 2021-08-03 10:33:06 +02:00

test if pack returns bytes in python2.

test if pack returns `bytes` in python2.
src/tests/common.py
PeterSurda (Migrated from github.com) commented 2021-08-03 10:40:03 +02:00

Try reverting this hunk

Try reverting this hunk
src/tests/test_networkgroup.py
PeterSurda (Migrated from github.com) commented 2021-08-03 10:40:48 +02:00

Retest after remongin this hunk.

Retest after remongin this hunk.
PeterSurda (Migrated from github.com) commented 2021-08-03 10:41:57 +02:00

should be only one section, with bytes, no distinguishing between python2 and python3

should be only one section, with bytes, no distinguishing between python2 and python3
PeterSurda (Migrated from github.com) commented 2021-08-03 10:45:00 +02:00

try making everything into bytes, we'll see if it still works.

try making everything into bytes, we'll see if it still works.
kdcis (Migrated from github.com) reviewed 2021-08-03 13:04:50 +02:00
src/protocol.py
kdcis (Migrated from github.com) commented 2021-08-03 13:04:50 +02:00

It works

It works
g1itch (Migrated from github.com) reviewed 2021-08-03 13:09:26 +02:00
src/protocol.py
g1itch (Migrated from github.com) commented 2021-08-03 13:09:25 +02:00

it's about str vs bytes here, to not insert a literal like b'', maybe bool makes sence but the condition is rather about the emptiness of the hostStandardFormat value as I see it

it's about str vs bytes here, to not insert a literal like `b''`, maybe bool makes sence but the condition is rather about the emptiness of the `hostStandardFormat` value as I see it
kdcis (Migrated from github.com) reviewed 2021-08-03 13:10:13 +02:00
src/protocol.py
kdcis (Migrated from github.com) commented 2021-08-03 13:10:13 +02:00

No it does not on python2,

It returns '\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\xff\xff\x7f\x00\x00\x01'

No it does not on python2, It returns ```'\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\xff\xff\x7f\x00\x00\x01'```
g1itch (Migrated from github.com) reviewed 2021-08-03 13:12:25 +02:00
src/tests/test_networkgroup.py
g1itch (Migrated from github.com) commented 2021-08-03 13:12:25 +02:00

I don't think you need this here. Can you show a corresponding failing check?

I don't think you need this here. Can you show a corresponding failing check?
kdcis (Migrated from github.com) reviewed 2021-08-03 13:15:04 +02:00
src/tests/test_networkgroup.py
kdcis (Migrated from github.com) commented 2021-08-03 13:15:04 +02:00

I was getting this error 'Found application file keys.dat in src dir'

So, I added tearDown() that fixed it

I was getting this error ```'Found application file keys.dat in src dir'``` So, I added tearDown() that fixed it
kdcis (Migrated from github.com) reviewed 2021-08-03 13:16:00 +02:00
src/tests/test_networkgroup.py
kdcis (Migrated from github.com) commented 2021-08-03 13:16:00 +02:00

I will retest removing it, once more

I will retest removing it, once more
g1itch (Migrated from github.com) reviewed 2021-08-03 13:19:27 +02:00
src/tests/common.py
g1itch (Migrated from github.com) commented 2021-08-03 13:19:27 +02:00

you've already got the pathmagic for this: the common module is used in both test suites

  • in the external tests we write from pybitmessage import state
  • but it the core tests it's still import state

I suppose we gonna switch to from . import state inside of the pybitmessage package and using another start script in top level dir, but not earlier than the circular import in the network package is solved.

you've already got the pathmagic for this: the `common` module is used in both test suites - in the external tests we write `from pybitmessage import state` - but it the core tests it's still `import state` I suppose we gonna switch to `from . import state` inside of the `pybitmessage` package and using another start script in top level dir, but not earlier than the circular import in the `network` package is solved.
g1itch (Migrated from github.com) reviewed 2021-08-03 13:21:31 +02:00
src/protocol.py
g1itch (Migrated from github.com) commented 2021-08-03 13:21:31 +02:00

what's called bytes in py3 in python2 is str
the py3 str analogue in python2 is unicode

what's called bytes in py3 in python2 is str the py3 str analogue in python2 is unicode
g1itch (Migrated from github.com) reviewed 2021-08-03 13:23:57 +02:00
src/protocol.py
g1itch (Migrated from github.com) commented 2021-08-03 13:23:57 +02:00

that's why you are not gonna get a portable code if you write str(data), instead you do data.encode('utf-8') # arg may be dropped in python3

that's why you are not gonna get a portable code if you write `str(data)`, instead you do `data.encode('utf-8') # arg may be dropped in python3`
g1itch (Migrated from github.com) reviewed 2021-08-03 13:25:20 +02:00
src/tests/test_networkgroup.py
g1itch (Migrated from github.com) commented 2021-08-03 13:25:20 +02:00

please post the link

please post the link
kdcis (Migrated from github.com) reviewed 2021-08-03 13:30:13 +02:00
src/tests/test_networkgroup.py
kdcis (Migrated from github.com) commented 2021-08-03 13:30:12 +02:00

https://github.com/Bitmessage/PyBitmessage/pull/1792/checks?sha=8d545fa066cdda420e1450ca248dc79ee1f74fba

I can't find the link for that logs, but it was for this commit

https://github.com/Bitmessage/PyBitmessage/pull/1792/checks?sha=8d545fa066cdda420e1450ca248dc79ee1f74fba I can't find the link for that logs, but it was for this commit
g1itch (Migrated from github.com) reviewed 2021-08-03 13:31:58 +02:00
src/tests/test_networkgroup.py
g1itch (Migrated from github.com) commented 2021-08-03 13:31:57 +02:00

I can't understand, why you have kept this? There is another copy of this test with additional samples in the test_protocol module

                                     |
                                     |
                                     V
I can't understand, why you have kept this? There is another copy of this test with additional samples in the `test_protocol` module ``` | | V ```
kdcis (Migrated from github.com) reviewed 2021-08-03 13:32:23 +02:00
src/tests/test_networkgroup.py
kdcis (Migrated from github.com) commented 2021-08-03 13:32:23 +02:00

Worked

Worked
kdcis (Migrated from github.com) reviewed 2021-08-03 13:32:32 +02:00
src/tests/test_networkgroup.py
kdcis (Migrated from github.com) commented 2021-08-03 13:32:32 +02:00

Removed

Removed
g1itch (Migrated from github.com) requested changes 2021-08-03 13:35:43 +02:00
g1itch (Migrated from github.com) left a comment

You've lost the logic of the tests refactoring. network group test goes to test_protocol

You've lost the logic of the tests refactoring. network group test goes to `test_protocol`
src/tests/test_networkgroup.py
g1itch (Migrated from github.com) commented 2021-08-03 13:34:48 +02:00

Probably it's because you should've remove this module, not add another bikeshed, see. below.

Probably it's because you should've remove this module, not add another bikeshed, see. below.
src/tests/test_protocol.py
@ -27,0 +67,4 @@
test_ip = '1.2.3.4'
self.assertEqual(b'\x01\x02', protocol.network_group(test_ip))
test_ip = '127.0.0.1'
g1itch (Migrated from github.com) commented 2021-08-03 13:33:20 +02:00

This is the new test_network_group()

This is the new `test_network_group()`
g1itch (Migrated from github.com) reviewed 2021-08-03 13:38:56 +02:00
src/tests/test_blindsig.py
g1itch (Migrated from github.com) commented 2021-08-03 13:38:55 +02:00

wrong order

wrong order
g1itch (Migrated from github.com) reviewed 2021-08-03 13:39:48 +02:00
src/tests/common.py
g1itch (Migrated from github.com) commented 2021-08-03 13:39:48 +02:00

You don't need this hunk either

You don't need this hunk either
kdcis (Migrated from github.com) reviewed 2021-08-03 13:40:27 +02:00
src/tests/test_blindsig.py
kdcis (Migrated from github.com) commented 2021-08-03 13:40:27 +02:00

There was code quality error popped up saying module level imports should be on top

There was code quality error popped up saying module level imports should be on top
g1itch (Migrated from github.com) reviewed 2021-08-03 13:44:58 +02:00
src/tests/test_blindsig.py
g1itch (Migrated from github.com) commented 2021-08-03 13:44:58 +02:00

nonsense: this is a library import, why did you place it before the standard library ones and why the for- import is before simple ones? you recently posted a link to pep8 ... so go read it yourself

nonsense: this is a library import, why did you place it before the standard library ones and why the for- import is before simple ones? you recently posted a link to pep8 ... so go read it yourself
kdcis (Migrated from github.com) reviewed 2021-08-03 13:50:09 +02:00
src/tests/test_protocol.py
@ -27,0 +67,4 @@
test_ip = '1.2.3.4'
self.assertEqual(b'\x01\x02', protocol.network_group(test_ip))
test_ip = '127.0.0.1'
kdcis (Migrated from github.com) commented 2021-08-03 13:50:08 +02:00

Okay, so I guess there's no need for test_networkgroup.py?
Should we remove it?

Edit: Removed it

Okay, so I guess there's no need for test_networkgroup.py? Should we remove it? Edit: Removed it
kdcis (Migrated from github.com) reviewed 2021-08-03 14:00:08 +02:00
src/tests/test_blindsig.py
kdcis (Migrated from github.com) commented 2021-08-03 14:00:08 +02:00

Ok, I reverted the changes
https://github.com/Bitmessage/PyBitmessage/pull/1792/checks?check_run_id=3230496739

Ok, I reverted the changes https://github.com/Bitmessage/PyBitmessage/pull/1792/checks?check_run_id=3230496739
kdcis (Migrated from github.com) reviewed 2021-08-03 14:07:14 +02:00
src/tests/test_blindsig.py
kdcis (Migrated from github.com) commented 2021-08-03 14:07:14 +02:00

Maybe it's because skip_python3() should be after the imports and not in between them.

But still, it's weird I was not getting this code quality error before

Maybe it's because ```skip_python3()``` should be after the imports and not in between them. But still, it's weird I was not getting this code quality error before
g1itch (Migrated from github.com) reviewed 2021-08-03 14:13:53 +02:00
src/tests/test_blindsig.py
g1itch (Migrated from github.com) commented 2021-08-03 14:13:52 +02:00

this branch is out of date, rebase

this branch is out of date, rebase
g1itch (Migrated from github.com) reviewed 2021-08-03 14:14:56 +02:00
src/tests/test_api.py
g1itch (Migrated from github.com) commented 2021-08-03 14:14:56 +02:00

This hunk was changed in v0.6

This hunk was changed in v0.6
g1itch (Migrated from github.com) reviewed 2021-08-03 14:15:47 +02:00
src/tests/test_blindsig.py
g1itch (Migrated from github.com) commented 2021-08-03 14:15:47 +02:00

there is no skip_python3() in v0.6, this hunk is not fit

there is no `skip_python3()` in v0.6, this hunk is not fit
kdcis (Migrated from github.com) reviewed 2021-08-03 14:18:37 +02:00
src/tests/test_blindsig.py
kdcis (Migrated from github.com) commented 2021-08-03 14:18:37 +02:00

Ok I found the issue, It was not properly rebased. I fixed it now.

Ok I found the issue, It was not properly rebased. I fixed it now.
kdcis (Migrated from github.com) reviewed 2021-08-03 14:19:11 +02:00
src/tests/test_blindsig.py
kdcis (Migrated from github.com) commented 2021-08-03 14:19:10 +02:00

Yep, Fixed now

Yep, Fixed now
kdcis (Migrated from github.com) reviewed 2021-08-03 14:21:16 +02:00
src/tests/common.py
kdcis (Migrated from github.com) commented 2021-08-03 14:21:16 +02:00

reverted

reverted
g1itch (Migrated from github.com) reviewed 2021-08-03 14:24:33 +02:00
src/protocol.py
g1itch (Migrated from github.com) commented 2021-08-03 14:24:33 +02:00

$ python
Python 2.7.18 (default, Aug 26 2020, 16:28:25)
[GCC 9.2.0] on linux2
Type "help", "copyright", "credits" or "license" for more information.

b'x7f\x00\x00\x01' == 'x7f\x00\x00\x01'
True

$ python Python 2.7.18 (default, Aug 26 2020, 16:28:25) [GCC 9.2.0] on linux2 Type "help", "copyright", "credits" or "license" for more information. >>> b'x7f\x00\x00\x01' == 'x7f\x00\x00\x01' True >>>
g1itch (Migrated from github.com) approved these changes 2021-08-03 14:27:22 +02:00
g1itch (Migrated from github.com) left a comment

OK. Now you need a Peter's review. I see this as a good material for squashing (;

OK. Now you need a Peter's review. I see this as a good material for squashing (;
PeterSurda (Migrated from github.com) approved these changes 2021-08-04 10:27:27 +02:00
g1itch commented 2021-08-04 16:58:55 +02:00 (Migrated from github.com)
Copy Link

Next time, please don't squash into one commit. Keep test changes separate from the code changes and write proper commit messages: what you've found in test results, what was changed to fix the issue. Your commit message is sick and not informative.

The proper message might be like this:

Enabled test_protocol and test_networkgroup on python3:
    
all the tests are in test_protocol.TestProtocol,
added test_checkIPv4Address(), test_checkIPv6Address(),
test_network_group() is extended with more samples,
separated test_check_local_socks() - this one still not works on python3.
    
That all essentially is by changing the literals in the corresponding
functions in the protocol module to bytes. Also b'\x01\x02'[0] == 0x1 in py3.
Next time, please don't squash into one commit. Keep test changes separate from the code changes and write proper commit messages: what you've found in test results, what was changed to fix the issue. Your commit message is sick and not informative. The proper message might be like this: ``` Enabled test_protocol and test_networkgroup on python3: all the tests are in test_protocol.TestProtocol, added test_checkIPv4Address(), test_checkIPv6Address(), test_network_group() is extended with more samples, separated test_check_local_socks() - this one still not works on python3. That all essentially is by changing the literals in the corresponding functions in the protocol module to bytes. Also b'\x01\x02'[0] == 0x1 in py3. ```
👍 1
g1itch commented 2021-08-11 15:07:53 +02:00 (Migrated from github.com)
Copy Link

Bad signature as well here

Bad signature as well here
kdcis commented 2021-08-12 09:30:21 +02:00 (Migrated from github.com)
Copy Link

Bad signature as well here

Updated verified commit.

> Bad signature as well here Updated verified commit.
g1itch commented 2021-08-12 15:31:51 +02:00 (Migrated from github.com)
Copy Link

Bad signature as well here

Updated verified commit.

Hmm, it looks like you've changed a GPG key...

> > Bad signature as well here > > Updated verified commit. Hmm, it looks like you've changed a GPG key...
kdcis commented 2021-08-12 15:36:14 +02:00 (Migrated from github.com)
Copy Link

Bad signature as well here

Updated verified commit.

Hmm, it looks like you've changed a GPG key...

Yes.

> > > Bad signature as well here > > > > > > Updated verified commit. > > Hmm, it looks like you've changed a GPG key... Yes.
g1itch (Migrated from github.com) requested changes 2021-08-13 13:12:54 +02:00
g1itch (Migrated from github.com) left a comment

Please rebase and change the commit message.

Please rebase and change the commit message.
This repo is archived. You cannot comment on pull requests.
Reviewers
No reviewers
PeterSurda
g1itch
Labels
Clear labels   
bug

Something isn't working as it's supposed to

  
build

Related to building or build system

  
dependencies

Pull requests that update a dependency file

  
developers

The issue is relevant mainly for developers rather than users

  
documentation

   
duplicate

   
enhancement

New feature

  
formatting

Code or UI text formatting

  
invalid

   
legal

   
mobile

Frontend for mobile devices (kivy)

  
obsolete

   
packaging

   
performance

   
protocol

Protocol change

  
question

   
refactoring

   
regression

Some issue, fixed in the past, appears again

  
security

   
test

   
translation

   
usability

   
wontfix
No Label
bug
build
dependencies
developers
documentation
duplicate
enhancement
formatting
invalid
legal
mobile
obsolete
packaging
performance
protocol
question
refactoring
regression
security
test
translation
usability
wontfix
Milestone
Clear milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
No Assignees
1 Participants
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: Bitmessage/PyBitmessage-2024-11-28#1792
No description provided.
Delete Branch "v0.6-test-networkgroup"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?