Skip to content

Conversation

g1itch
Copy link
Collaborator

@g1itch g1itch commented Jul 21, 2021

Hello!

Let's introduce some simple compatibility tests, based on a sample data and well known values. I wish them to cover the implementations of all the functionality, defined in the protocol. So that an alternative implementation can take the samples to check it's compatibility.

It's also helpful for the ongoing python3 porting. Because most of the developers are not writing any tests. And I've already found a feature in highlevelcrypto module, which I don't yet understand, but I think that such incompatibilities is better to reveal soon. The test shows that not all the subset of pyelliptic is used in highlevelcrypto in compatible way with python3.

@g1itch g1itch added enhancement New feature refactoring developers The issue is relevant mainly for developers rather than users documentation test labels Jul 21, 2021
@g1itch
Copy link
Collaborator Author

g1itch commented Jul 21, 2021

And of course the check's gonna fail.

@g1itch g1itch force-pushed the compatibility branch 2 times, most recently from ab52048 to 321d234 Compare July 26, 2021 22:13
@g1itch g1itch added the protocol Protocol change label Jul 27, 2021
@g1itch
Copy link
Collaborator Author

g1itch commented Jul 27, 2021

The test shows that not all the subset of pyelliptic is used in highlevelcrypto in compatible way with python3.

It's even worse: apparently the pyelliptic is not python3 compatible. It's just not covered with tests.

@g1itch
Copy link
Collaborator Author

g1itch commented Jul 28, 2021

And I've already found a feature in highlevelcrypto module, which I don't yet understand, but I think that such incompatibilities is better to reveal soon.

solved it in the recent code: https://github.com/g1itch/PyBitmessage/runs/3182720003#step:8:70

@g1itch g1itch removed the protocol Protocol change label Jul 28, 2021
@g1itch g1itch force-pushed the compatibility branch 2 times, most recently from 0c2f1b6 to 4448900 Compare August 3, 2021 12:08
@g1itch g1itch force-pushed the compatibility branch 4 times, most recently from 5982784 to 6f4eb38 Compare August 18, 2021 13:30
@g1itch g1itch force-pushed the compatibility branch 3 times, most recently from 0f5c0cd to ba9ec7c Compare August 20, 2021 11:41
@g1itch g1itch force-pushed the compatibility branch 2 times, most recently from fbb6409 to fe691a5 Compare September 3, 2021 14:27
@g1itch g1itch mentioned this pull request Nov 2, 2021
@PeterSurda
Copy link
Member

Maybe this shouldn't be called "Compatiblity tests" as most of it is network subsystem refactoring. Could you perhaps put the (smaller) protocol refactoring into a separate PR, and just keep this for the network subsystem? Then the protocol refactoring can be merged earlier and faster.

@PeterSurda
Copy link
Member

maybe also "random" refactoring separately.

@g1itch
Copy link
Collaborator Author

g1itch commented Nov 15, 2021

Maybe this shouldn't be called "Compatiblity tests" as most of it is network subsystem refactoring. Could you perhaps put the (smaller) protocol refactoring into a separate PR, and just keep this for the network subsystem? Then the protocol refactoring can be merged earlier and faster.

I see, the rest of this branch is currently a complicated changeset merged from different branches with pretty controversial changes mixed with a few tests. I'm slowly reading the other part of the implementation in search of a pieces, which I can isolate clearly and cover by tests. Unsuccessfully so far.

Maybe I'll rebase the branch, if I found a way to write cleaner set of tests for the significant portion of the protocol. But the overall idea of those "Compatiblity tests" is covering the protocol structures, excluding those related to crypto operations (the #1796 part).

@g1itch
Copy link
Collaborator Author

g1itch commented Nov 15, 2021

maybe also "random" refactoring separately.

Yes, the random is probably closer to the refactoring of the crypto operations. But this particular approach is a bit problematic, so I don't merge those changes into the crypto branch and so far neither I see a way to isolate the related changes into a separate PR. So I'm going to rename the branch and replace it by the further changes when they are ready.

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

Labels

developers The issue is relevant mainly for developers rather than users documentation enhancement New feature refactoring test

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants