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

BROKEN unity tests of MG stack #1041

Open
lorbax opened this issue Jul 8, 2024 · 13 comments
Open

BROKEN unity tests of MG stack #1041

lorbax opened this issue Jul 8, 2024 · 13 comments
Assignees

Comments

@lorbax
Copy link
Collaborator

lorbax commented Jul 8, 2024

In main.rs there are a lot of unity tests which are commented, and many other are not working

@plebhash

This comment was marked as outdated.

@plebhash
Copy link
Collaborator

plebhash commented Jul 9, 2024

bringing over @Fi3 's message:

I had some free time so I started sketching out something:
https://github.com/demand-open-source/demand-easy-sv2
this is the lib
https://github.com/demand-open-source/demand-test-data-collector
this the proxy
for now I made it print on std out in json format
it work a little bit and the crash
for very old issue in the sv2 serde lib
we need to fix them otherwise we will encouter them also in the MG as soon as we will start to use extensevly

user@user ~/s/demand-test-data-collector (master) [101]> SERVER=0.0.0.0:20000 CLIENT=localhost:20022 cargo run
{"Common":{"SetupConnection":{"protocol":0,"min_version":2,"max_version":2,"flags":1,"endpoint_host":[58,58,49],"endpoint_port":20022,"vendor":[],"hardware_version":[],"firmware":[],"device_id":[98,98,58,58,83,79,76,79,58,58,98,99,49,113,120,121,50,107,103,100,121,103,106,114,115,113,116,122,113,50,110,48,121,114,102,50,52,57,51,112,56,51,107,107,102,106,104,120,48,119,108,104]}}}
{"Common":{"SetupConnectionSuccess":{"used_version":2,"flags":1}}}
{"Mining":{"SetNewPrevHash":{"channel_id":1,"job_id":1,"prev_hash":[47,80,94,178,202,185,27,239,30,212,198,100,223,11,237,208,159,243,103,143,103,138,0,0,0,0,0,0,0,0,0,0],"min_ntime":1716474877,"nbits":386094576}}}
{"Mining":{"OpenStandardMiningChannel":{"request_id":10,"user_identity":[],"nominal_hash_rate":1424131.9,"max_target":[0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,55,2,0,0,0,0,0,0]}}}
{"Mining":{"OpenStandardMiningChannelSuccess":{"request_id":10,"channel_id":1,"target":[92,53,131,220,180,93,152,171,116,151,29,232,107,51,80,183,139,164,192,28,55,3,243,120,152,23,59,164,246,1,0,0],"extranonce_prefix":[0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,1],"group_channel_id":0}}}
thread 'main' panicked at /home/user/.cargo/registry/src/index.crates.io-6f17d22bba15001f/serde_sv2-1.0.0/src/primitives/sequences/option.rs:245:13:
explicit panic

this is just a stupid demo to see if the lib that I'm developing can be used and how easy it is to use
it seem good for now
all the proxy stay in 75 line of code
https://github.com/demand-open-source/demand-test-data-collector/blob/master/src/main.rs

@Shourya742

This comment was marked as outdated.

@lorbax

This comment was marked as outdated.

@Shourya742

This comment was marked as outdated.

@lorbax

This comment was marked as outdated.

@plebhash

This comment was marked as outdated.

@GitGab19
Copy link
Collaborator

PR #949 which was solving this has been closed some weeks ago.
@lorbax do you think we should keep this issue open for now?

@jbesraa
Copy link
Contributor

jbesraa commented Aug 21, 2024

I think we decided to focus on integration tests for now and not spend our limited resources on message generator

@GitGab19
Copy link
Collaborator

GitGab19 commented Aug 21, 2024

I think we decided to focus on integration tests for now and not spend our limited resources on message generator

Yes I know, but I preferred to ask @lorbax since the MG still remains a tool with the purpose of testing alternative implementations of SV2. So this issue could still be valid

@lorbax
Copy link
Collaborator Author

lorbax commented Aug 23, 2024

@GitGab19 @jbesraa
The purpose of MG is to test compatibility against SRI, and it allows you to test other implementations of Stratum V2. This is a thing that you cannot do with integration test and to replace it with integration tests leads to a loss of functionality in my opinion.
But even if you test messages between template provider (in C++) and this repo (so remaining within SRI), you will eventually do something similar to MG.

@lorbax
Copy link
Collaborator Author

lorbax commented Aug 23, 2024

About the issue I don't know what you want to do. If you close it the unity tests remain broken and you simply will ignore this fact.

@GitGab19
Copy link
Collaborator

@GitGab19 @jbesraa The purpose of MG is to test compatibility against SRI, and it allows you to test other implementations of Stratum V2. This is a thing that you cannot do with integration test and to replace it with integration tests leads to a loss of functionality in my opinion. But even if you test messages between template provider (in C++) and this repo (so remaining within SRI), you will eventually do something similar to MG.

It seems we got some misunderstanding.

  1. The new integration tests framework will be used to do integration tests (especially on our CI), instead of the current MG.
  2. The MG itself still remains a valid tool that should be used for its initial purpose: test interoperability between SV2 implementations

Given 2), MG won't be completely gone, indeed its focus will be again its original one. For this reason I was asking you @lorbax what do you want to do about this issue. From my perspective, I don't see any reason to close this, and to have closed the PR that was fixing it

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: In Progress 🏗️
6 participants