Add basic Qt wallet test #9974

pull ryanofsky wants to merge 6 commits into bitcoin:master from ryanofsky:pr/qt-test changing 9 files +206 −38
  1. ryanofsky commented at 9:56 pm on March 10, 2017: member

    This adds a basic unit test for the Qt send coins dialog. I have more changes (for a future PR) that extend this to cover new Qt bumpfee code in #9697.

    Previously, I brought up the topic of qt unit tests in #9817 (comment) and #9697 (review).

  2. Make test_bitcoin.cpp compatible with Qt Test framework 
    Move Boost.Test main function and global overrides to a new test_bitcoin_main.cpp file.
    91e303595b
  3. Make qt test compatible with TestChain100Setup framework 
    Reset global state after rpc tests, and remove unnecessary ECC initialization
to prevent assert error if it is initialized twice.
    cc9503cec9
  4. fanquake added the label Tests on Mar 10, 2017
  5. MarcoFalke added the label GUI on Mar 10, 2017
  6. luke-jr commented at 1:34 am on March 11, 2017: member
    I almost wonder if it might be useful to have a RPC wrapper for the GUI (possibly in addition to this) so we can just use the RPC tests with it as well.
  7. fanquake commented at 2:00 am on March 11, 2017: member

    Failed with:

     0==============================================
 1   Bitcoin Core 0.14.99: src/test-suite.log
 2==============================================
 3# TOTAL: 2
 4# PASS:  1
 5# SKIP:  0
 6# XFAIL: 0
 7# FAIL:  1
 8# XPASS: 0
 9# ERROR: 0
10.. contents:: :depth: 2
11FAIL: qt/test/test_bitcoin-qt
12=============================
13This application failed to start because it could not find or load the Qt platform plugin "xcb"
14in "".
15Reinstalling the application may fix this problem.
  8. laanwj commented at 8:39 am on March 11, 2017: member
    Concept ACK
  9. MarcoFalke commented at 9:31 am on March 11, 2017: member
    Concept ACK for sure, as we basically don’t have any gui tests right now. In the long term I’d prefer a solution that runs on a higher abstraction level. Your current approach is pretty much mimicking functional tests on a low (unit test) level.
  10. ryanofsky commented at 9:51 am on March 11, 2017: member

    I’d prefer a solution that runs on a higher abstraction level. Your current approach is pretty much mimicking functional tests on a low (unit test) level.

    I don’t think I understand. I’m not doing anything original here, just following the approach of http://doc.qt.io/qt-5/qttestlib-tutorial3-example.html.

    Is it that instead of the writing a test that sends qt events, you would prefer to see a test that starts an x server and sends keyboard and mouse events?

  11. jonasschnelli commented at 11:36 am on March 11, 2017: contributor
    I really like this. Concept ACK.
  12. MarcoFalke commented at 11:54 am on March 11, 2017: member

    Is it that instead of the writing a test that sends qt events, you would prefer to see a test that starts an x server and sends keyboard and mouse events?

    Indeed. I was dreaming of a gui test framework where you just press a “record this” button. Then records discrete actions (e.g. “Press: Receive tab”, “Press: Copy address”, other gui: “Press: Send tab”, “Insert: $address”, …) instead of the exact mouse and keyboard movements, such that it works on different screen resolutions. Though, we also need a way to wait for the gui to catch up on longer tasks such as importmulti with rescan, so I don’t think this is a weekend project.

    On Sat, Mar 11, 2017 at 12:36 PM, Jonas Schnelli notifications@github.com wrote:

    I really like this. Concept ACK.

    — You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/bitcoin/bitcoin/pull/9974#issuecomment-285861292, or mute the thread https://github.com/notifications/unsubscribe-auth/AGGmv2gRizL47AbknwIa11DdwdLFQFBrks5rkoc4gaJpZM4MZ6Pd .

  13. in src/qt/test/test_main.cpp: in 3feca350d1 outdated 
    44@@ -46,7 +45,7 @@ int main(int argc, char *argv[])
45 
46     // Don't remove this, it's needed to access
47     // QCoreApplication:: in the tests
    fanquake commented at 3:24 am on March 12, 2017:
    Comment needs removing/updating?
    ryanofsky commented at 5:25 pm on March 15, 2017:
    It’s still accurate, but I expanded the comment in f4d28fbe1b8041ce358d80ecb0e1fa074eee359c.
  14. fanquake commented at 3:25 am on March 12, 2017: member
    Still failing Travis with the same issue, @theuni should probably take a look here.
  15. in src/qt/test/test_main.cpp: in 3feca350d1 outdated 
    63@@ -65,7 +64,11 @@ int main(int argc, char *argv[])
64     CompatTests test4;
65     if (QTest::qExec(&test4) != 0)
66         fInvalid = true;
67+#ifdef ENABLE_WALLET
68+    WalletTests test5;
69+    if (QTest::qExec(&test5) != 0)
    MarcoFalke commented at 11:40 am on March 12, 2017:
    style nit: We use braces now; Always.
    ryanofsky commented at 5:26 pm on March 15, 2017:
    Added in b61b34c89dbe3946e21147dcfe620a4a6ff536ed
  16. ryanofsky force-pushed on Mar 15, 2017
  17. Add braces to if statements in Qt test_main b61b34c89d
  18. Add simple qt wallet test sending a transaction 2754ef1c4a
  19. Add new test_bitcoin-qt static library dependencies 
    Avoids following error when qt is statically linked into the test binary, as on
travis:

This application failed to start because it could not find or load the Qt platform plugin "xcb"
in "".
    9e6817ed11
  20. Enable xvfb in travis to allow running test_bitcoin-qt 
    Avoids following error:

QXcbConnection: Could not connect to display
    9576b015a1
  21. ryanofsky force-pushed on Mar 15, 2017
  22. ryanofsky force-pushed on Mar 15, 2017
  23. ryanofsky force-pushed on Mar 15, 2017
  24. ryanofsky commented at 5:27 pm on March 15, 2017: member
    Finally got test to pass on travis.
  25. jonasschnelli commented at 1:31 pm on March 16, 2017: contributor

    A lock is missing.

     0diff --git a/src/qt/test/wallettests.cpp b/src/qt/test/wallettests.cpp
 1index 81b358e..d63f4ec 100644
 2--- a/src/qt/test/wallettests.cpp
 3+++ b/src/qt/test/wallettests.cpp
 4@@ -73,6 +73,7 @@ void WalletTests::walletTests()
 5     test.CreateAndProcessBlock({}, GetScriptForRawPubKey(test.coinbaseKey.GetPubKey()));
 6     bitdb.MakeMock();
 7     CWallet wallet("wallet_test.dat");
 8+    LOCK(wallet.cs_wallet);
 9     bool firstRun;
10     wallet.LoadWallet(firstRun);
11     wallet.SetAddressBook(test.coinbaseKey.GetPubKey().GetID(), "", "receive");

    How could this pass travis with the missing lock?

    Will properly ACK after apply the diff above.

    Question: is it possible to create a screenshot (or a bitmap/JPG/PNG) of the UI elements you open? A such form of an UI export would be great. We could add this then to a CI and have plenty of screenshots from various operating systems in order to verify the UI correctness.

  26. ryanofsky commented at 1:52 pm on March 16, 2017: member

    A lock is missing.

    Will add. I’m not sure the way the lock is acquired in the diff is exactly right, because cs_main is supported to be acquired before cs_wallet, and some wallet methods acquire both.

    How could this pass travis with the missing lock?

    Locally, the test works fine for me with the missing lock. On travis, every configuration we have either specifies NO_QT or NO_WALLET, or omits RUN_TESTS, so neither PaymentServerTests code nor this new WalletTests code currently executes.

    Question: is it possible to create a screenshot (or a bitmap/JPG/PNG) of the UI elements you open? A such form of an UI export would be great. We could add this then to a CI and have plenty of screenshots from various operating systems in order to verify the UI correctness.

    Something like this should be possible. To help debug this change, I frequently added widget.show(); QEventLoop().exec(); calls to be able to see and interact with individual test widgets, so it should be possible to capture images also (maybe with QWidget::grab?)

  27. ryanofsky referenced this in commit 739f19ca96 on Mar 16, 2017
  28. ryanofsky force-pushed on Mar 16, 2017
  29. ryanofsky commented at 6:35 pm on March 16, 2017: member

    Fixed locking bug in 739f19ca969c2ae351e18d2bbb424237830f54f1

    Squashed 739f19ca969c2ae351e18d2bbb424237830f54f1 -> 9576b015a107b98fc950c574ed01d993b388d7c9 (pr/qt-test.7 -> pr/qt-test.8)

  30. MarcoFalke commented at 0:17 am on March 17, 2017: member
    utACK 9576b015a107b98fc950c574ed01d993b388d7c9
  31. jonasschnelli commented at 1:27 pm on March 17, 2017: contributor
    Tested ACK 9576b015a107b98fc950c574ed01d993b388d7c9
  32. jonasschnelli merged this on Mar 17, 2017
  33. jonasschnelli closed this on Mar 17, 2017
  34. jonasschnelli referenced this in commit b9f930b383 on Mar 17, 2017
  35. jnewbery commented at 4:30 pm on March 17, 2017: member

    This is great. Hopefully I’ll have coverage running soon so we can see how much extra code you’ve managed to exercise.

    Post-merge tested ACK https://github.com/bitcoin/bitcoin/commit/9576b015a107b98fc950c574ed01d993b388d7c9.

  36. paveljanik commented at 2:34 pm on March 18, 2017: contributor

    BTW, compiling this is not possible with gcc-4.8.5, the default gcc on openSUSE 42.2:

    0/usr/include/boost/signals2/connection.hpp: In function 'uint256 {anonymous}::SendCoins(CWallet&, SendCoinsDialog&, const CBitcoinAddress&, CAmount)':
1/usr/include/boost/signals2/connection.hpp:234:7: error: 'boost::signals2::scoped_connection::scoped_connection(const boost::signals2::scoped_connection&)' is private
2       scoped_connection(const scoped_connection &other);
3       ^
4qt/test/wallettests.cpp:47:6: error: within this context
5     });
6      ^
7Makefile:7991: recipe for target 'qt/test/qt_test_test_bitcoin_qt-wallettests.o' failed
8make[2]: *** [qt/test/qt_test_test_bitcoin_qt-wallettests.o] Error 1
  37. ryanofsky commented at 2:48 pm on March 18, 2017: member

    Possible fix might be to change:

    0boost::signals2::scoped_connection c = wallet.NotifyTransactionChanged.connect(...);

    to:

    0boost::signals2::scoped_connection c(wallet.NotifyTransactionChanged.connect(...));
  38. paveljanik commented at 4:24 pm on March 18, 2017: contributor
    Hmm, all gcc’s on openSUSE 42.2, ie. 4.8.5, 5.3.1 and 6.2.1, fail on the current master…
  39. paveljanik commented at 4:43 pm on March 18, 2017: contributor
    With #9974 (comment) change, the build is OK. Will you please PR it?
  40. paveljanik commented at 9:00 pm on March 18, 2017: contributor

    And another issue here. On plain Debian Jessie, with the gcc version 4.9.2 (Debian 4.9.2-10), this doesn’t compile with:

     0qt/test/wallettests.cpp:34:6: error: no matching function for call to 'QTimer::singleShot(int, Qt::TimerType, {anonymous}::ConfirmSend()::<lambda()>)'
 1     });
 2      ^
 3qt/test/wallettests.cpp:34:6: note: candidates are:
 4In file included from /usr/include/x86_64-linux-gnu/qt5/QtCore/QTimer:1:0,
 5                 from ./qt/sendcoinsdialog.h:13,
 6                 from qt/test/wallettests.cpp:7:
 7/usr/include/x86_64-linux-gnu/qt5/QtCore/qtimer.h:81:17: note: static void QTimer::singleShot(int, const QObject*, const char*)
 8     static void singleShot(int msec, const QObject *receiver, const char *member);
 9                 ^
10/usr/include/x86_64-linux-gnu/qt5/QtCore/qtimer.h:81:17: note:   no known conversion for argument 2 from 'Qt::TimerType' to 'const QObject*'
11/usr/include/x86_64-linux-gnu/qt5/QtCore/qtimer.h:82:17: note: static void QTimer::singleShot(int, Qt::TimerType, const QObject*, const char*)
12     static void singleShot(int msec, Qt::TimerType timerType, const QObject *receiver, const char *member);
13                 ^
14/usr/include/x86_64-linux-gnu/qt5/QtCore/qtimer.h:82:17: note:   candidate expects 4 arguments, 3 provided
15Makefile:7991: recipe for target 'qt/test/qt_test_test_bitcoin_qt-wallettests.o' failed
16make[2]: *** [qt/test/qt_test_test_bitcoin_qt-wallettests.o] Error 1
  41. ryanofsky referenced this in commit d5046e72f4 on Mar 20, 2017
  42. ryanofsky referenced this in commit b5bec4e330 on Mar 20, 2017
  43. practicalswift referenced this in commit 5096d0eb37 on Apr 27, 2017
  44. practicalswift referenced this in commit a1a425773f on Apr 27, 2017
  45. MarcoFalke referenced this in commit f199b8a33d on Oct 2, 2017
  46. HashUnlimited referenced this in commit baa2dc272a on Feb 28, 2018
  47. HashUnlimited referenced this in commit 83214daac5 on Feb 28, 2018
  48. PastaPastaPasta referenced this in commit 05fbeed513 on Mar 10, 2019
  49. PastaPastaPasta referenced this in commit 0040b24ad5 on Mar 10, 2019
  50. PastaPastaPasta referenced this in commit deb42a0124 on Mar 11, 2019
  51. PastaPastaPasta referenced this in commit 52047783ea on Mar 11, 2019
  52. PastaPastaPasta referenced this in commit 1b8b2c8703 on Mar 12, 2019
  53. PastaPastaPasta referenced this in commit 8616f29006 on Mar 13, 2019
  54. UdjinM6 referenced this in commit ca3d42f280 on Mar 13, 2019
  55. PastaPastaPasta referenced this in commit 1e535c89bb on Mar 13, 2019
  56. PastaPastaPasta referenced this in commit 23be81805c on Mar 14, 2019
  57. PastaPastaPasta referenced this in commit e81ee57887 on Mar 14, 2019
  58. PastaPastaPasta referenced this in commit 4e7857f9d3 on Mar 15, 2019
  59. PastaPastaPasta referenced this in commit 66f9bb2016 on Mar 16, 2019
  60. PastaPastaPasta referenced this in commit 71208dafe7 on Apr 3, 2019
  61. PastaPastaPasta referenced this in commit 63f992d9e9 on Apr 3, 2019
  62. PastaPastaPasta referenced this in commit 90db870b3a on Apr 5, 2019
  63. PastaPastaPasta referenced this in commit aee4472ace on May 6, 2019
  64. PastaPastaPasta referenced this in commit d3c59cfea4 on Jan 17, 2020
  65. PastaPastaPasta referenced this in commit 2c244c2cd3 on Jan 22, 2020
  66. PastaPastaPasta referenced this in commit 41678a382d on Jan 22, 2020
  67. barrystyle referenced this in commit 67ba2b915c on Jan 22, 2020
  68. PastaPastaPasta referenced this in commit 715426b41b on Jan 29, 2020
  69. PastaPastaPasta referenced this in commit 6e829d96fe on Jan 29, 2020
  70. PastaPastaPasta referenced this in commit 27a4f30f5f on Jan 31, 2020
  71. PastaPastaPasta referenced this in commit 6d8738eb9e on Jan 31, 2020
  72. ckti referenced this in commit bae751ab06 on Mar 28, 2021
  73. gades referenced this in commit ba241e9bfb on Jun 30, 2021
  74. DrahtBot locked this on Sep 8, 2021


ryanofsky luke-jr fanquake laanwj MarcoFalke jonasschnelli jnewbery paveljanik

Labels
GUI Tests

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-11-17 15:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me