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

Run bitcoin_test-qt under minimal QPA platform #10142

Merged
merged 1 commit into from Apr 10, 2017

Conversation

Projects
None yet
6 participants
@ryanofsky
Copy link
Contributor

ryanofsky commented Apr 3, 2017

Fixes broken "make check" reported by @TheBlueMatt in #10110

Fix was suggested and initially implemented by @theuni in #10117 (comment)

Run bitcoin_test-qt under minimal QPA platform
Fixes broken "make check" reported by Matt Corallo <git@bluematt.me> in
#10110

Fix was suggested and initially implemented by
Cory Fields <cory-nospam-@coryfields.com> in
#10117 (comment)
@laanwj

This comment has been minimized.

Copy link
Member

laanwj commented Apr 3, 2017

Concept ACK

@fanquake fanquake added the Tests label Apr 3, 2017

@sipa

This comment has been minimized.

Copy link
Member

sipa commented Apr 4, 2017

Concept ACK

@laanwj

This comment has been minimized.

Copy link
Member

laanwj commented Apr 6, 2017

@TheBlueMatt can you check if this solves your issue #10110?

@theuni
Copy link
Member

theuni left a comment

Concept ACK and ACK after fixing up the configure check.

@@ -68,6 +68,18 @@ QModelIndex FindTx(const QAbstractItemModel& model, const uint256& txid)
}

//! Simple qt wallet tests.
//
// Test widgets can be debugged interactively calling show() on them and
// manually running the event loop, e.g.:

This comment has been minimized.

@theuni

theuni Apr 6, 2017

Member

Can this be done automatically based on the platform selected? Or does it require manual placement of the .show()s? If not, that would at least make it so that you can avoid a recompile for debug cycles.

This comment has been minimized.

@ryanofsky

ryanofsky Apr 6, 2017

Author Contributor

Can this be done automatically

It would probably be possible to test code that would show all widgets. So far I've found it useful just to show selected widgets when debugging.

This comment has been minimized.

@laanwj

laanwj Apr 7, 2017

Member

As I understand it the issues troubleshooted here are rare enough that it's fine to have the developer insert his own .show()s at appropriate places. It's just useful to have this comment in that case.

@@ -130,6 +130,8 @@ AC_DEFUN([BITCOIN_QT_CONFIGURE],[
if test "x$bitcoin_cv_need_acc_widget" = "xyes"; then
_BITCOIN_QT_CHECK_STATIC_PLUGINS([Q_IMPORT_PLUGIN(AccessibleFactory)], [-lqtaccessiblewidgets])
fi
_BITCOIN_QT_CHECK_STATIC_PLUGINS([Q_IMPORT_PLUGIN(QMinimalIntegrationPlugin)],[-lqminimal])

This comment has been minimized.

@theuni

theuni Apr 6, 2017

Member

We need to make it so that this doesn't signal failed qt detection if libqminimal isn't found. It'd be fine if it were just test_bitcoin-qt disabled, but this would also disable the bitcoin-qt build which has no dependency on the lib.

This comment has been minimized.

@ryanofsky

ryanofsky Apr 6, 2017

Author Contributor

We need to make it so that this doesn't signal failed qt detection if libqminimal isn't found.

What should happen if minimal isn't found, and why we can't just make it a requirement? I looked into trying making it make it optional, but it seemed like a mess, see my comment #10117 (comment)

@theuni

This comment has been minimized.

Copy link
Member

theuni commented Apr 6, 2017

@laanwj I can confirm that it does, I'm seeing the issue locally as well.

@jonasschnelli

This comment has been minimized.

Copy link
Member

jonasschnelli commented Apr 9, 2017

Tested ACK on debian 8 with Qt5.3 headless:
bf10264

@laanwj

This comment has been minimized.

Copy link
Member

laanwj commented Apr 10, 2017

Thanks for testing. Then I'm going to merge this to prevent test failures for affected devs; the build system integration can be improved later.

@laanwj laanwj merged commit bf10264 into bitcoin:master Apr 10, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

laanwj added a commit that referenced this pull request Apr 10, 2017

Merge #10142: Run bitcoin_test-qt under minimal QPA platform
bf10264 Run bitcoin_test-qt under minimal QPA platform (Russell Yanofsky)

Tree-SHA512: 35782f0d7e4dcdc27d991d5a10fcffbd2d201139293fe7917ef6f7cd7ae4d3a162ebc21f83266d821ae3bad86f62d947b047bb317f6c5899df4d6bcb4c957157

ryanofsky added a commit to ryanofsky/bitcoin that referenced this pull request Jun 2, 2017

Remove xvfb configuration from travis
Should no longer be needed after bitcoin#10142:

bf10264 "Run bitcoin_test-qt under minimal QPA platform"
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.