-
Notifications
You must be signed in to change notification settings - Fork 141
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
Enhance SetupConnection Handling and Implement Protocol-Specific Flag Checks #918
Conversation
Bencher
Click to view all benchmark results
Bencher - Continuous Benchmarking View Public Perf Page Docs | Repo | Chat | Help |
Bencher
Click to view all benchmark results
Bencher - Continuous Benchmarking View Public Perf Page Docs | Repo | Chat | Help |
Bencher
Click to view all benchmark results
Bencher - Continuous Benchmarking View Public Perf Page Docs | Repo | Chat | Help |
Bencher
Click to view all benchmark results
Bencher - Continuous Benchmarking View Public Perf Page Docs | Repo | Chat | Help |
let flag_avaiable = 0b_0000_0000_0000_0000_0000_0000_0000_0000; | ||
fn test_check_flags() { | ||
let protocol = Protocol::MiningProtocol; | ||
let flag_available = 0b_0000_0000_0000_0000_0000_0000_0000_0001; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why you changed this test?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thought of it as a way ensuring that check_flags works when both available and required flags are the same for MiningProtocol and JobDeclarationProtocol.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Fi3 when I did the revert I keep getting this error:
test setup_connection::test::test_check_flag ... FAILED
test setup_connection::test::test_has_work_selection ... ok
test setup_connection::test::test_set_requires_std_job ... ok
failures:
---- setup_connection::test::test_check_flag stdout ----
thread 'setup_connection::test::test_check_flag' panicked at v2/subprotocols/common-messages/src/setup_connection.rs:408:9:
assertion failed: SetupConnection::check_flags(protocol, flag_avaiable, flag_required)
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
failures:
setup_connection::test::test_check_flag
```
any idea on how I can go about it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Fi3 I managed to fix the error, I think you can review again and give feedback
@xyephy looks like Rustfmt CI is broken, can you do |
@xyephy did you explore the MG tests? I'm wondering if this behavior is already covered in our MG suite, or should we add a new one? |
for this issue I hadn't done that, let me explore/review then give you feedback. |
@xyephy any progress on writing the MG tests for this one? |
I'm 80% done on this |
closing in favor of #1035 |
This PR works on addressing the discussions in issue #853