Compatibility tests #1788

2021-07-21T18:39:44+02:00

g1itch commented

2021-07-21 18:39:44 +02:00

(Migrated from github.com)

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.

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](https://github.com/g1itch/PyBitmessage/runs/3116252583?check_suite_focus=true) 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 commented

2021-07-21 18:41:10 +02:00

(Migrated from github.com)

And of course the check's gonna fail.

g1itch commented

2021-07-28 00:01:00 +02:00

(Migrated from github.com)

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.

> 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 commented

2021-07-28 16:23:00 +02:00

(Migrated from github.com)

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

> And I've already found [a feature](https://github.com/g1itch/PyBitmessage/runs/3116252583?check_suite_focus=true) 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

PeterSurda commented

2021-11-15 08:11:37 +01:00

(Migrated from github.com)

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 commented

2021-11-15 08:12:18 +01:00

(Migrated from github.com)

maybe also "random" refactoring separately.

g1itch commented

2021-11-15 13:37:32 +01:00

(Migrated from github.com)

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).

> 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](https://pybitmessage-test.readthedocs.io/en/doc/protocol.html#common-structures), excluding those related to crypto operations (the #1796 part).

g1itch commented

2021-11-15 13:44:33 +01:00

(Migrated from github.com)

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.

> 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.

This repo is archived. You cannot comment on pull requests.

No reviewers