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

Add BitcoinApplication & RPCConsole tests #11625

Merged
merged 2 commits into from Jan 9, 2019

Conversation

Projects
None yet
@ryanofsky
Copy link
Contributor

ryanofsky commented Nov 7, 2017

Add test coverage for Qt initialization code & basic RPC console functionality

Motivation for this change was a bug in #11603 which existing tests failed to catch.

@fanquake

This comment has been minimized.

Copy link
Member

fanquake commented Nov 7, 2017

Travis failures:

In file included from /usr/include/qt4/QtGui/QLineEdit:1:0,
                 from qt/test/apptests.cpp:20:
/usr/include/qt4/QtGui/qlineedit.h: In function ‘void {anonymous}::TestRpcCommand(RPCConsole*)’:
/usr/include/qt4/QtGui/qlineedit.h:198:10: error: ‘void QLineEdit::returnPressed()’ is protected
     void returnPressed();
          ^
qt/test/apptests.cpp:34:29: error: within this context
     lineEdit->returnPressed();
                             ^
make[2]: *** [qt/test/qt_test_test_bitcoin_qt-apptests.o] Error 1
FAIL: qt/test/test_bitcoin-qt
=============================
********* Start testing of AppTests *********
Config: Using QtTest library 5.7.1, Qt 5.7.1 (x86_64-little_endian-lp64 static release build; by GCC 4.8.4)
PASS   : AppTests::initTestCase()
QWARN  : AppTests::appTests() Backing up GUI settings to "/tmp/test_bitcoin-qt_1510018491_71945/regtest/guisettings.ini.bak"
QWARN  : AppTests::appTests() QFont::setPointSizeF: Point size <= 0 (-1.000000), must be greater than 0
QWARN  : AppTests::appTests() QFont::setPointSizeF: Point size <= 0 (-1.000000), must be greater than 0
QWARN  : AppTests::appTests() QFont::setPointSizeF: Point size <= 0 (-1.000000), must be greater than 0
QWARN  : AppTests::appTests() QFont::setPointSizeF: Point size <= 0 (-1.000000), must be greater than 0
QWARN  : AppTests::appTests() QFont::setPointSizeF: Point size <= 0 (-1.000000), must be greater than 0
QWARN  : AppTests::appTests() QFont::setPointSizeF: Point size <= 0 (-1.000000), must be greater than 0
QWARN  : AppTests::appTests() QFont::setPointSizeF: Point size <= 0 (-1.000000), must be greater than 0
QWARN  : AppTests::appTests() QFont::setPointSizeF: Point size <= 0 (-1.000000), must be greater than 0
QDEBUG : AppTests::appTests() requestInitialize : Requesting initialize
QDEBUG : AppTests::appTests() initialize : Running initialization in thread
QDEBUG : AppTests::appTests() initializeResult : Initialization result:  true
QWARN  : AppTests::appTests() Platform customization: "other"
========= Received signal, dumping stack ==============
========= End of stack trace ==============
QFATAL : AppTests::appTests() Received signal 11
         Function time: 267ms Total time: 272ms
FAIL!  : AppTests::appTests() Received a fatal error.
   Loc: [Unknown file(0)]
Totals: 1 passed, 1 failed, 0 skipped, 0 blacklisted, 284ms
********* Finished testing of AppTests *********
@promag
Copy link
Member

promag left a comment

LGTM. Some nits below.

Show resolved Hide resolved src/qt/test/apptests.cpp Outdated
Show resolved Hide resolved src/qt/test/apptests.cpp Outdated
Show resolved Hide resolved src/qt/test/apptests.h Outdated

@ryanofsky ryanofsky force-pushed the ryanofsky:pr/apptest branch 4 times, most recently from 5be833c to 4bf5ecd Nov 7, 2017

@ryanofsky
Copy link
Contributor

ryanofsky left a comment

Had to make some fixes for travis: 5be833c -> 4bf5ecd (pr/apptest.1 -> pr/apptest.3), there was QSystemTrayIcon segfault happening with an older version of qt5 and there were compile errors with qt4.

Show resolved Hide resolved src/qt/test/apptests.cpp Outdated
Show resolved Hide resolved src/qt/test/apptests.h Outdated
Show resolved Hide resolved src/qt/test/apptests.cpp Outdated

@ryanofsky ryanofsky force-pushed the ryanofsky:pr/apptest branch 5 times, most recently from 4bf5ecd to e94e344 Nov 8, 2017

@ryanofsky
Copy link
Contributor

ryanofsky left a comment

Had to make some more #include fixes, but travis is actually fixed now. Updated 4bf5ecd -> e94e344 (pr/apptest.3 -> pr/apptest.4)

Show resolved Hide resolved src/qt/test/apptests.cpp Outdated
@promag
Copy link
Member

promag left a comment

Some nits otherwise LGTM. Will test locally.

Show resolved Hide resolved src/qt/bitcoingui.cpp
Show resolved Hide resolved src/qt/test/apptests.cpp
Show resolved Hide resolved src/qt/test/apptests.cpp
@promag

This comment has been minimized.

Copy link
Member

promag commented Nov 9, 2017

Any hint?

make clean && make

...

  CXXLD    qt/test/test_bitcoin-qt
Undefined symbols for architecture x86_64:
  "MacNotificationHandler::instance()", referenced from:
      Notificator::Notificator(QString const&, QSystemTrayIcon*, QWidget*) in libbitcoinqt.a(qt_libbitcoinqt_a-notificator.o)
      Notificator::notify(Notificator::Class, QString const&, QString const&, QIcon const&, int) in libbitcoinqt.a(qt_libbitcoinqt_a-notificator.o)
  "MacNotificationHandler::hasUserNotificationCenterSupport()", referenced from:
      Notificator::Notificator(QString const&, QSystemTrayIcon*, QWidget*) in libbitcoinqt.a(qt_libbitcoinqt_a-notificator.o)
  "MacNotificationHandler::showNotification(QString const&, QString const&)", referenced from:
      Notificator::notify(Notificator::Class, QString const&, QString const&, QIcon const&, int) in libbitcoinqt.a(qt_libbitcoinqt_a-notificator.o)
  "MacDockIconHandler::cleanup()", referenced from:
      BitcoinGUI::~BitcoinGUI() in libbitcoinqt.a(qt_libbitcoinqt_a-bitcoingui.o)
  "MacDockIconHandler::setMainWindow(QMainWindow*)", referenced from:
      BitcoinGUI::createTrayIconMenu() in libbitcoinqt.a(qt_libbitcoinqt_a-bitcoingui.o)
  "MacDockIconHandler::dockMenu()", referenced from:
      BitcoinGUI::createTrayIconMenu() in libbitcoinqt.a(qt_libbitcoinqt_a-bitcoingui.o)
  "MacDockIconHandler::instance()", referenced from:
      BitcoinGUI::BitcoinGUI(PlatformStyle const*, NetworkStyle const*, QWidget*) in libbitcoinqt.a(qt_libbitcoinqt_a-bitcoingui.o)
      BitcoinGUI::createTrayIconMenu() in libbitcoinqt.a(qt_libbitcoinqt_a-bitcoingui.o)
  "MacDockIconHandler::setIcon(QIcon const&)", referenced from:
      BitcoinGUI::BitcoinGUI(PlatformStyle const*, NetworkStyle const*, QWidget*) in libbitcoinqt.a(qt_libbitcoinqt_a-bitcoingui.o)
ld: symbol(s) not found for architecture x86_64
clang: error: linker command failed with exit code 1 (use -v to see invocation)
make[2]: *** [qt/test/test_bitcoin-qt] Error 1
make[2]: *** Waiting for unfinished jobs....
make[1]: *** [all-recursive] Error 1
make: *** [all-recursive] Error 1

@MarcoFalke MarcoFalke added the GUI label Nov 10, 2017

@ryanofsky ryanofsky force-pushed the ryanofsky:pr/apptest branch from e94e344 to 55fe983 Nov 28, 2017

@jonasschnelli

This comment has been minimized.

Copy link
Member

jonasschnelli commented Nov 29, 2017

Have the same compile issue (clang):

Undefined symbols for architecture x86_64:
  "MacNotificationHandler::instance()", referenced from:
      Notificator::Notificator(QString const&, QSystemTrayIcon*, QWidget*) in libbitcoinqt.a(qt_libbitcoinqt_a-notificator.o)
      Notificator::notifyMacUserNotificationCenter(Notificator::Class, QString const&, QString const&, QIcon const&) in libbitcoinqt.a(qt_libbitcoinqt_a-notificator.o)
  "MacNotificationHandler::hasUserNotificationCenterSupport()", referenced from:
      Notificator::Notificator(QString const&, QSystemTrayIcon*, QWidget*) in libbitcoinqt.a(qt_libbitcoinqt_a-notificator.o)
  "MacNotificationHandler::showNotification(QString const&, QString const&)", referenced from:
      Notificator::notifyMacUserNotificationCenter(Notificator::Class, QString const&, QString const&, QIcon const&) in libbitcoinqt.a(qt_libbitcoinqt_a-notificator.o)
  "MacDockIconHandler::cleanup()", referenced from:
      BitcoinGUI::~BitcoinGUI() in libbitcoinqt.a(qt_libbitcoinqt_a-bitcoingui.o)
  "MacDockIconHandler::setMainWindow(QMainWindow*)", referenced from:
      BitcoinGUI::createTrayIconMenu() in libbitcoinqt.a(qt_libbitcoinqt_a-bitcoingui.o)
  "MacDockIconHandler::dockMenu()", referenced from:
      BitcoinGUI::createTrayIconMenu() in libbitcoinqt.a(qt_libbitcoinqt_a-bitcoingui.o)
  "MacDockIconHandler::instance()", referenced from:
      BitcoinGUI::BitcoinGUI(PlatformStyle const*, NetworkStyle const*, QWidget*) in libbitcoinqt.a(qt_libbitcoinqt_a-bitcoingui.o)
      BitcoinGUI::createTrayIconMenu() in libbitcoinqt.a(qt_libbitcoinqt_a-bitcoingui.o)
  "MacDockIconHandler::setIcon(QIcon const&)", referenced from:
      BitcoinGUI::BitcoinGUI(PlatformStyle const*, NetworkStyle const*, QWidget*) in libbitcoinqt.a(qt_libbitcoinqt_a-bitcoingui.o)
ld: symbol(s) not found for architecture x86_64
clang: error: linker command failed with exit code 1 (use -v to see invocation)
make[2]: *** [qt/test/test_bitcoin-qt] Error 1
make[1]: *** [all-recursive] Error 1

Looks like an OSX issue.

@ryanofsky

This comment has been minimized.

Copy link
Contributor

ryanofsky commented Nov 30, 2017

Looking into the link error, but @jonasschnelli do you have an idea why the build might be failing for you even though the apple target succeeds on travis?

@ryanofsky

This comment has been minimized.

Copy link
Contributor

ryanofsky commented Nov 30, 2017

Looking into the link error, but @jonasschnelli do you have an idea why the build might be failing for you even though the apple target succeeds on travis?

Seems like the reason is that travis doesn't build tests for the apple target (it is just running make deploy).

I also think I see the reason for the error. BITCOIN_MM files seem to get directly linked into the qt/bitcoin-qt executable instead of being part of qt/libbitcoinqt.a. I will try to move them there.

@ryanofsky ryanofsky force-pushed the ryanofsky:pr/apptest branch from 55fe983 to b67d524 Nov 30, 2017

@promag

This comment has been minimized.

Copy link
Member

promag commented Nov 30, 2017

I can dig a little to see if I can find a fix.

@ryanofsky
Copy link
Contributor

ryanofsky left a comment

Updated 55fe983 -> b67d524 (pr/apptest.5 -> pr/apptest.6) to fix the link error. I was able to reproduce it by building test_bitcoin-qt in a travis docker image, and confirmed that it now links.

Show resolved Hide resolved src/qt/bitcoingui.cpp
Show resolved Hide resolved src/qt/test/apptests.cpp
Show resolved Hide resolved src/qt/test/apptests.cpp
@jb55

jb55 approved these changes Dec 30, 2017

Copy link
Contributor

jb55 left a comment

Linux Tested ACK b67d524

@laanwj

This comment has been minimized.

Copy link
Member

laanwj commented Jan 30, 2018

Travis was passing, but I just re-triggered it (to test it on top of master), now https://travis-ci.org/bitcoin/bitcoin/jobs/309721182 :(

Strange:

0.01s$ if [ "$CHECK_DOC" = 1 -a "$TRAVIS_EVENT_TYPE" = "pull_request" ]; then contrib/devtools/lint-all.sh; fi
contrib/devtools/lint-python.sh: 10: contrib/devtools/lint-python.sh: flake8: not found
^---- failure generated from contrib/devtools/lint-python.sh
@MarcoFalke

This comment has been minimized.

Copy link
Member

MarcoFalke commented Jan 30, 2018

@laanwj It is a travis bug :( They use the source code from master (merged with this pull) but the .travis.yml of only the pull...

Needs rebase

@ryanofsky ryanofsky force-pushed the ryanofsky:pr/apptest branch 2 times, most recently from 328eb86 to 412fb2d Feb 1, 2018

@ryanofsky ryanofsky force-pushed the ryanofsky:pr/apptest branch from 9f4a8af to 1bbfce8 Oct 22, 2018

@DrahtBot DrahtBot removed the Needs rebase label Oct 22, 2018

Show resolved Hide resolved src/qt/test/apptests.h Outdated

@ryanofsky ryanofsky force-pushed the ryanofsky:pr/apptest branch from 1bbfce8 to 71510b9 Oct 23, 2018

@ryanofsky ryanofsky force-pushed the ryanofsky:pr/apptest branch from 71510b9 to cb5a114 Oct 25, 2018

@DrahtBot DrahtBot removed the Needs rebase label Oct 25, 2018

@ryanofsky ryanofsky force-pushed the ryanofsky:pr/apptest branch from cb5a114 to e6cb33a Nov 12, 2018

@DrahtBot DrahtBot removed the Needs rebase label Nov 12, 2018

@Sjors

This comment has been minimized.

Copy link
Member

Sjors commented Nov 20, 2018

Tests pass for me on macOS 10.14.1, with and without --disable-bip70.

Shouldn't this be run as part of make check? Otherwise, maybe add them to Travis in a followup?

@MarcoFalke

This comment has been minimized.

Copy link
Member

MarcoFalke commented Dec 6, 2018

Could move the first commit to a separate pull request, since that is what needs rebase all the time?

@ryanofsky ryanofsky force-pushed the ryanofsky:pr/apptest branch from e6cb33a to 7cca59d Dec 7, 2018

@DrahtBot DrahtBot removed the Needs rebase label Dec 7, 2018

@jonasschnelli

This comment has been minimized.

Copy link
Member

jonasschnelli commented Jan 4, 2019

@ryanofsky: interested to pick this up again?

ryanofsky added some commits Nov 7, 2017

Add BitcoinApplication & RPCConsole tests
Add test coverage for Qt initialization code & basic RPC console functionality.

@ryanofsky ryanofsky force-pushed the ryanofsky:pr/apptest branch from 7cca59d to 7e4bd19 Jan 4, 2019

@DrahtBot DrahtBot removed the Needs rebase label Jan 5, 2019

@ryanofsky

This comment has been minimized.

Copy link
Contributor

ryanofsky commented Jan 8, 2019

@ryanofsky: interested to pick this up again?

There was a minor conflict with #15000 last week, but I am maintaining this (and think it probably could be merged).

@jonasschnelli

This comment has been minimized.

Copy link
Member

jonasschnelli commented Jan 9, 2019

Tested ACK ca20b65

I don't ran into problems when running on macOS with QT_QPA_PLATFORM=cocoa, I can see then GUI window opening and closing.

Did also ran without issues on Ubuntu Bionic with minimal and xcb Platform.

@ryanofsky any reasons why – on Ubuntu – I see the window opening/closing during the AddressBookTests but not during the AppTests?

@MarcoFalke MarcoFalke merged commit 7e4bd19 into bitcoin:master Jan 9, 2019

2 checks passed

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

MarcoFalke added a commit that referenced this pull request Jan 9, 2019

Merge #11625: Add BitcoinApplication & RPCConsole tests
7e4bd19 Add BitcoinApplication & RPCConsole tests (Russell Yanofsky)
ca20b65 Move BitcoinApplication to header so it can be tested (Russell Yanofsky)

Pull request description:

  Add test coverage for Qt initialization code & basic RPC console functionality

  Motivation for this change was a bug in #11603 which existing tests failed to catch.

Tree-SHA512: f66546ffc84b8e07679c66a73b265023fbf6a0cb8f24f1606a5fcae2dd3b4dc7b2c6d26c69dedcec53398a26ef17c4d5fb28c055698fa6e45e89aa2995cefe2f
@ryanofsky

This comment has been minimized.

Copy link
Contributor

ryanofsky commented Jan 9, 2019

@ryanofsky any reasons why – on Ubuntu – I see the window opening/closing during the AddressBookTests but not during the AppTests?

That would be an unexpected bug. I'm not seeing any windows shown by default with 699d0bd (current master) running qt/test/test_bitcoin-qt. I do see windows if I set QT_QPA_PLATFORM=xcb (as expected).

You may want to experiment with unset DISPLAY && src/qt/test/test_bitcoin-qt. This should pass and not show any windows.

domob1812 added a commit to domob1812/namecore that referenced this pull request Jan 14, 2019

Merge branch 'bitcoin' into auxpow
Updated regtest chain parameters and tests for the Segwit changes in
Namecoin after  upstream fab17e8.

Removed BIP34Hash from chain parameters as that is never used at all
in Namecoin (only the height is).

Replaced BEGIN/END macros in auxpow code.  At the same time, removed all
other code explicitly dealing with endian-ness in favour of methods
encoding data independently from the system's byte order.

Note that the Qt unit test fails with this commit, but it also fails
upstream in Bitcoin in the same way.  That is likely due to an upstream
bug in bitcoin/bitcoin#11625.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment