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.

Sign up for GitHub

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

Jump to bottom

qa: Fix service flag comparison check in rpc_net test #16936

Closed
wants to merge 1 commit into from
Closed

qa: Fix service flag comparison check in rpc_net test #16936

wants to merge 1 commit into from

Conversation

luke-jr
Copy link
Member

@luke-jr luke-jr commented Sep 22, 2019

#16850 was basically completely broken...

@luke-jr
Copy link
Member Author

luke-jr commented Sep 22, 2019

This fixes it, but the stupid linters don't like it:

test/functional/test_framework/messages.py:46: unused variable 'NODE_GETUTXO' (60% confidence)

Can I just get rid of the linter, or is there another approach that would be preferred?

@fanquake fanquake added the Tests label Sep 22, 2019
@promag
Copy link
Member

promag commented Sep 22, 2019

You could add to test/lint/lint-python-dead-code-whitelist?

@luke-jr luke-jr force-pushed the fix_servicesnames_test branch 2 times, most recently from 3300669 to 9a0421d Compare September 22, 2019 22:51
@luke-jr
Copy link
Member Author

luke-jr commented Sep 22, 2019

There, got linter happy.

@theStack
Copy link
Contributor

ACK 78487ef

That was quite an interesting chain of coincidences and fails; the fact that the tested servicesflags (0x809) didn't contain any hexadecimal symbols a-f covered the wrong base for the hexstring-to-integer conversion, which in turn was covered by a too weak assertion policy in assert_net_servicesnames(), also allowing more names in the list than just the ones represented in the flags. An additional assertion bin(servicesflags).count('1') == len(servicesnames) would have also solved the problem, but the PR approach, getting rid of all the if statements, is more generic and flexible.

luke-jr added a commit to bitcoinknots/bitcoin that referenced this pull request Sep 23, 2019
@luke-jr
Bugfix: QA: Fix service flag comparison check in rpc_net test
4801f33 
Github-Pull: bitcoin#16936
Rebased-From: a308fe4
@maflcko maflcko added this to the 0.19.0 milestone Sep 23, 2019
@maflcko
Copy link
Member

maflcko commented Sep 23, 2019

I wouldn't be surprised if other users also stepped into this trap. The field should probably be a normal json number, not a hex encoded string version of the int.

Concept ACK anyway

@luke-jr
Copy link
Member Author

luke-jr commented Sep 23, 2019

Yes, agree it should be a Number ideally.

@maflcko
Copy link
Member

maflcko commented Sep 23, 2019

unsigned ACK 78487ef (looked at the diff on GitHub)

@DrahtBot
Copy link
Contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #16961 (test: Remove python dead code linter by laanwj)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

maflcko
maflcko reviewed Sep 25, 2019
View reviewed changes
test/lint/lint-python-dead-code-whitelist
@@ -12,6 +12,8 @@ InvalidOPIFConstruction # unused class (test/functional/data/invalid_txs.py)
_.is_compressed # unused property (test/functional/test_framework/key.py)
legacy # unused variable (test/functional/test_framework/address.py)
msg_generic # unused class (test/functional/test_framework/messages.py)
NODE_BLOOM # actually used (test/functional/test_framework/messages.py)
NODE_GETUTXO # actually used (test/functional/test_framework/messages.py)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This file was deleted in master. Mind to remove those changes again and force push? (No need to rebase, if you don't want to)

@DrahtBot
Copy link
Contributor

Needs rebase

@laanwj laanwj changed the title Bugfix: QA: Fix service flag comparison check in rpc_net test qa: Fix service flag comparison check in rpc_net test Sep 26, 2019
@laanwj laanwj mentioned this pull request Sep 30, 2019
Merged
@laanwj
Copy link
Member

laanwj commented Sep 30, 2019

Closing in favor of #16991

@laanwj laanwj removed this from the 0.19.0 milestone Sep 30, 2019
@fanquake fanquake closed this Sep 30, 2019
laanwj added a commit that referenced this pull request Sep 30, 2019
@laanwj
Merge #16991: qa: Fix service flag comparison check in rpc_net test (…
c3a8e09 
…luke-jr)

9c23ebd qa: Fix service flag comparison check in rpc_net test (Luke Dashjr)

Pull request description:

  Rebase of #16936

ACKs for top commit:
  darosior:
    ACK 9c23ebd

Tree-SHA512: 74f287740403da1040ab1e235ef6eba4e304f3ee5d57a3b25d1e2e1f2f982d256528d398a4d6cb24ba393798e680a8f46cd7dae54ed84ab2c747e96288f1f884
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Dec 16, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@maflcko maflcko maflcko left review comments

Assignees
No one assigned
Labels
Tests
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@luke-jr @promag @theStack @maflcko @DrahtBot @laanwj @fanquake