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

qt test: Don't bind to regtest port #233

Merged
merged 1 commit into from
Mar 8, 2021

Conversation

achow101
Copy link
Member

@achow101 achow101 commented Mar 1, 2021

The qt tests don't need to bind to the regtest port. By not binding, it will no longer conflict with existing regtest instances and the tests will run as normal.

Fixes #10

Copy link
Member

@jarolrod jarolrod left a comment

Choose a reason for hiding this comment

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

ACK a9d1ddd

Confirming the behavior cited in #10 on master. Also confirming that this PR fixes it.

Tested on macOS 11.2 Qt 5.15.2 using the QT_QPA_PLATFORM=cocoa ./test_bitcoin-qt work around.

I setup a node running bitcoin-qt -regtest, then setup two instances of the test to run at the same time. Everything ran smoothly and the blocking behavior was not encountered.

@hebasto hebasto added the Tests label Mar 6, 2021
@hebasto
Copy link
Member

hebasto commented Mar 6, 2021

I think this change prevents an opportunity to test Qt signals and UI response when new block/tx arrives.

(that, of course, will require to spin up another node)

The qt tests don't need to bind to the regtest port. By not binding, it
will no longer conflict with existing regtest instances and the tests
will run as normal.
@achow101
Copy link
Member Author

achow101 commented Mar 6, 2021

I think this change prevents an opportunity to test Qt signals and UI response when new block/tx arrives.

(that, of course, will require to spin up another node)

In that case, -port can be overridden. In any case, I don't think that we should reject this just because there might be a change in the future that conflicts with it.

@maflcko
Copy link
Contributor

maflcko commented Mar 7, 2021

cr ACK e21276a

didn't test

@@ -54,6 +54,13 @@ int main(int argc, char* argv[])

NodeContext node_context;
std::unique_ptr<interfaces::Node> node = interfaces::MakeNode(&node_context);
gArgs.ForceSetArg("-listen", "0");
Copy link
Member

Choose a reason for hiding this comment

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

Passing 0 to port lets the system assign a free port number. This fixes the issue while allowing for future GUI tests that involve interactions with another node.

Suggested change
gArgs.ForceSetArg("-listen", "0");
gArgs.ForceSetArg("-port", "0");

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think that is necessary at this time. Additionally, I don't think doing this would be useful. In this hypothetical test, the test framework needs to know what port is being used, so I don't think this would help.

Copy link
Member

Choose a reason for hiding this comment

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

true

Copy link
Member

@jarolrod jarolrod left a comment

Choose a reason for hiding this comment

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

re-ACK e21276a, tested on macOS 11.2

@maflcko maflcko merged commit 2067f9e into bitcoin-core:master Mar 8, 2021
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Mar 8, 2021
e21276a qt test: Don't bind to regtest port (Andrew Chow)

Pull request description:

  The qt tests don't need to bind to the regtest port. By not binding, it will no longer conflict with existing regtest instances and the tests will run as normal.

  Fixes #10

ACKs for top commit:
  MarcoFalke:
    cr ACK e21276a
  jarolrod:
    re-ACK e21276a, tested on macOS 11.2

Tree-SHA512: 5a269ee043f9aff7900e092c166de71912a2bf86ebe2982b3fb0e26bdebfb91869ee5d0f62082fd608c1288bfb7981f6c8647e504b11176711d7fec993a09164
gwillen pushed a commit to ElementsProject/elements that referenced this pull request Jun 28, 2022
@bitcoin-core bitcoin-core locked as resolved and limited conversation to collaborators Aug 16, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use random free port for test_bitcoin-qt
4 participants