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] [tests] Clarify address book error messages, add tests #12830

Merged
merged 4 commits into from Apr 25, 2018
Merged
Changes from 1 commit
Commits
File filter...
Filter file types
Jump to…
Jump to file or symbol
Failed to load files and symbols.

Always

Just for now

Prev

[tests] [qt] Add tests for address book manipulation via EditAddressD…

…ialog

Also modifies corresponding QT code to allow for use within test cases.
  • Loading branch information...
jamesob committed Mar 29, 2018
commit 5109fc4a9cb2cbd73c33197fb9129e1413ab051b
@@ -12,11 +12,13 @@ TEST_QT_MOC_CPP = \

if ENABLE_WALLET
TEST_QT_MOC_CPP += \
qt/test/moc_addressbooktests.cpp \
qt/test/moc_paymentservertests.cpp \
qt/test/moc_wallettests.cpp
endif

TEST_QT_H = \
qt/test/addressbooktests.h \
qt/test/compattests.h \
qt/test/rpcnestedtests.h \
qt/test/uritests.h \
@@ -45,6 +47,7 @@ qt_test_test_bitcoin_qt_SOURCES = \
$(TEST_BITCOIN_H)
if ENABLE_WALLET
qt_test_test_bitcoin_qt_SOURCES += \
qt/test/addressbooktests.cpp \
qt/test/paymentservertests.cpp \
qt/test/wallettests.cpp \
wallet/test/wallet_test_fixture.cpp
@@ -38,7 +38,7 @@ class AddressBookPage : public QDialog
ForEditing /**< Open address book for editing */
};

explicit AddressBookPage(const PlatformStyle *platformStyle, Mode mode, Tabs tab, QWidget *parent);
explicit AddressBookPage(const PlatformStyle *platformStyle, Mode mode, Tabs tab, QWidget *parent = 0);
~AddressBookPage();

void setModel(AddressTableModel *model);
@@ -30,7 +30,7 @@ class EditAddressDialog : public QDialog
EditSendingAddress
};

explicit EditAddressDialog(Mode mode, QWidget *parent);
explicit EditAddressDialog(Mode mode, QWidget *parent = 0);
~EditAddressDialog();

void setModel(AddressTableModel *model);
@@ -0,0 +1,143 @@
#include <qt/test/addressbooktests.h>
#include <qt/test/util.h>
#include <test/test_bitcoin.h>

#include <interfaces/node.h>
#include <qt/addressbookpage.h>
#include <qt/addresstablemodel.h>
#include <qt/editaddressdialog.h>
#include <qt/callback.h>
#include <qt/optionsmodel.h>
#include <qt/platformstyle.h>
#include <qt/qvalidatedlineedit.h>
#include <qt/walletmodel.h>

#include <key.h>
#include <pubkey.h>
#include <key_io.h>
#include <wallet/wallet.h>

#include <QTimer>
#include <QMessageBox>

namespace
{

/**
* Fill the edit address dialog box with data, submit it, and ensure that
* the resulting message meets expectations.
*/
void EditAddressAndSubmit(
EditAddressDialog* dialog,
const QString& label, const QString& address, QString expected_msg)
{
QString warning_text;

dialog->findChild<QLineEdit*>("labelEdit")->setText(label);
dialog->findChild<QValidatedLineEdit*>("addressEdit")->setText(address);

ConfirmMessage(&warning_text, 5);
dialog->accept();
QCOMPARE(warning_text, expected_msg);
}

/**
* Test adding various send addresses to the address book.
*
* There are three cases tested:
*
* - new_address: a new address which should add as a send address successfully.
* - existing_s_address: an existing sending address which won't add successfully.
* - existing_r_address: an existing receiving address which won't add successfully.
*
* In each case, verify the resulting state of the address book and optionally
* the warning message presented to the user.
*/
void TestAddAddressesToSendBook()
{
TestChain100Setup test;
CWallet wallet("mock", WalletDatabase::CreateMock());
bool firstRun;
wallet.LoadWallet(firstRun);

auto build_address = [&wallet]() {

This comment has been minimized.

Copy link
@jimpo

jimpo Apr 25, 2018

Contributor

commit: [tests] [qt] Add tests for address book manipulation via EditAddressDialog

nit: Any particular reason for this to be a lambda expression instead of a regular function in the anonymous namespace?

CKey key;
key.MakeNewKey(true);
CTxDestination dest(GetDestinationForKey(
key.GetPubKey(), wallet.m_default_address_type));

return std::make_pair(dest, QString::fromStdString(EncodeDestination(dest)));
};

CTxDestination r_key_dest, s_key_dest;

// Add a preexisting "receive" entry in the address book.
QString preexisting_r_address;
QString r_label("already here (r)");

// Add a preexisting "send" entry in the address book.
QString preexisting_s_address;
QString s_label("already here (s)");

// Define a new address (which should add to the address book successfully).
QString new_address;

std::tie(r_key_dest, preexisting_r_address) = build_address();
std::tie(s_key_dest, preexisting_s_address) = build_address();
std::tie(std::ignore, new_address) = build_address();

{
LOCK(wallet.cs_wallet);
wallet.SetAddressBook(r_key_dest, r_label.toStdString(), "receive");
wallet.SetAddressBook(s_key_dest, s_label.toStdString(), "send");
}

auto check_addbook_size = [&wallet](int expected_size) {

This comment has been minimized.

Copy link
@jimpo

jimpo Apr 25, 2018

Contributor

nit: Also could be a regular function.

QCOMPARE(static_cast<int>(wallet.mapAddressBook.size()), expected_size);
};

// We should start with the two addresses we added earlier and nothing else.
check_addbook_size(2);

// Initialize relevant QT models.
std::unique_ptr<const PlatformStyle> platformStyle(PlatformStyle::instantiate("other"));

This comment has been minimized.

Copy link
@jimpo

jimpo Apr 25, 2018

Contributor

commit: [tests] [qt] Add tests for address book manipulation via EditAddressDialog

nit: camelCase vs snake_case of local variables in this function is inconsistent (r_key_dest vs platformStyle). I think snake_case is preferred for new code.

auto node = interfaces::MakeNode();
OptionsModel optionsModel(*node);
AddWallet(&wallet);
WalletModel walletModel(std::move(node->getWallets()[0]), *node, platformStyle.get(), &optionsModel);
RemoveWallet(&wallet);
EditAddressDialog editAddressDialog(EditAddressDialog::NewSendingAddress);
editAddressDialog.setModel(walletModel.getAddressTableModel());

EditAddressAndSubmit(
&editAddressDialog, QString("uhoh"), preexisting_r_address,
QString(
"Address \"%1\" already exists as a receiving address with label "
"\"%2\" and so cannot be added as a sending address."
).arg(preexisting_r_address).arg(r_label));

check_addbook_size(2);

EditAddressAndSubmit(
&editAddressDialog, QString("uhoh, different"), preexisting_s_address,
QString(
"The entered address \"%1\" is already in the address book with "
"label \"%2\"."
).arg(preexisting_s_address).arg(s_label));

check_addbook_size(2);

// Submit a new address which should add successfully - we expect the
// warning message to be blank.
EditAddressAndSubmit(
&editAddressDialog, QString("new"), new_address, QString(""));

check_addbook_size(3);
}

} // namespace

void AddressBookTests::addressBookTests()
{
TestAddAddressesToSendBook();
}
@@ -0,0 +1,15 @@
#ifndef BITCOIN_QT_TEST_ADDRESSBOOKTESTS_H
#define BITCOIN_QT_TEST_ADDRESSBOOKTESTS_H

#include <QObject>
#include <QTest>

class AddressBookTests : public QObject
{
Q_OBJECT

private Q_SLOTS:
void addressBookTests();
};

#endif // BITCOIN_QT_TEST_ADDRESSBOOKTESTS_H
@@ -13,6 +13,7 @@
#include <qt/test/compattests.h>

#ifdef ENABLE_WALLET
#include <qt/test/addressbooktests.h>
#include <qt/test/paymentservertests.h>
#include <qt/test/wallettests.h>
#endif
@@ -99,6 +100,10 @@ int main(int argc, char *argv[])
if (QTest::qExec(&test5) != 0) {
fInvalid = true;
}
AddressBookTests test6;
if (QTest::qExec(&test6) != 0) {
fInvalid = true;
}
#endif

fs::remove_all(pathTemp);
@@ -204,6 +204,8 @@ class WalletModel : public QObject
QString getWalletName() const;

bool isMultiwallet();

AddressTableModel* getAddressTableModel() const { return addressTableModel; }

This comment has been minimized.

Copy link
@jimpo

jimpo Apr 25, 2018

Contributor

commit: [tests] [qt] Add tests for address book manipulation via EditAddressDialog

This method is already defined on line 139 but without const. I think the non-const variant is actually better because it returns a non-const pointer. (While technically allowed, it feels wrong to me to return a non-const pointer to a member from a const method).

private:
std::unique_ptr<interfaces::Wallet> m_wallet;
std::unique_ptr<interfaces::Handler> m_handler_status_changed;
ProTip! Use n and p to navigate between commits in a pull request.
You can’t perform that action at this time.