Add Qt tests for wallet spends
ClosedPublic
Actions

Authored by jasonbcox on Feb 12 2019, 01:19.

Details

Reviewers

deadalnix
Fabien

Group Reviewers

Restricted Project

Commits

rSTAGINGc3f35428fd2e: Add Qt tests for wallet spends
rABCc3f35428fd2e: Add Qt tests for wallet spends

Summary

A few code changes were needed to accompany the test:

Adding setObjectName() calls for a few Qt controls to make them easily accessible from the test.

Calling contextMenu->popup() instead of contextMenu->exec() to open the transaction list context menu without blocking the test thread.

Opening the context menu at the contextualMenu event point rather than the cursor position (this change was not strictly needed to make the test work, but is more correct).

Updating the bumped transaction row with showTransaction=true instead of false. This is needed to prevent the bumped tx from being hidden, so the last part of the test which attempts to bump the bumped tx can work. (Technically this change is a more general bugfix not limited to the testing environment, but the bug doesn't happen outside of the testing environment because in the full Qt client, a queued NotifyTransactionChanged notification causes the row to be updated twice, first with showTransaction=false, then immediately after with showTransaction=true.)

Backport of Core PR10420
https://github.com/bitcoin/bitcoin/pull/10420/files
Completes T512

Test Plan

make check

Diff Detail

Repository

rABC Bitcoin ABC

Branch

master

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 4962
Build 7987: Bitcoin ABC Buildbot (legacy)
Build 7986: arc lint + arc unit

Event Timeline

jasonbcox created this revision.Feb 12 2019, 01:19

Herald added a reviewer: Restricted Project. · View Herald TranscriptFeb 12 2019, 01:19

Herald added a subscriber: schancel. · View Herald Transcript

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing started.

jasonbcox added inline comments.Feb 12 2019, 01:21

src/qt/test/wallettests.cpp
37	The difference here is because we do not support Qt4 (see https://github.com/bitcoin/bitcoin/pull/10098/files for reference to the introduction to `callback.h` which I don't believe to be necessary)

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing passed.

Harbormaster completed remote builds in B4962: Diff 7307.Feb 12 2019, 01:34

I can see this is a dependency for T417 in the task list but fail to see how they are linked together; is there another intermediate backport that needs to build on top of this one ?

src/qt/test/wallettests.cpp
17	Not used, can be removed
24	dito
26	dito
36	There is no case where these new parameters are actually used. They are introduced in the PR for testing the BumpFee feature. Is there addition required for later backports ?
42	braces
src/qt/transactionview.cpp
144	Even if I think it is a good practice to name the elements, here the name is set in order to let the BumpFee function find the view in the original PR, and actually has no other usage.
160	Dito, this one seems to have no use even in the original PR

This revision now requires changes to proceed.Feb 12 2019, 08:29

jasonbcox marked 7 inline comments as done.Feb 12 2019, 19:07

jasonbcox added inline comments.

src/qt/test/wallettests.cpp
36	I see two reasons for pulling this in: Reduce merge conflicts. This is a definite improvement over the old ConfirmSend. We can utilize this if we wish.
src/qt/transactionview.cpp
144	Interesting. I figured all objects required names, but these are the only two objects with them and both are unused.

Fixed according to feedback

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing started.

I can see this is a dependency for T417 in the task list but fail to see how they are linked together; is there another intermediate backport that needs to build on top of this one ?

This appears to be true. From a shallow look, it appeared this was a required dependency, but as I dug more into this backport, it's clear that T417 can be done without it. Regardless, I think it will be helpful pulling in more small changes like this to reduce merge conflicts in the future.

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing passed.

Harbormaster completed remote builds in B4967: Diff 7313.Feb 12 2019, 19:23

Fabien accepted this revision.Feb 12 2019, 20:21

This revision is now accepted and ready to land.Feb 12 2019, 20:21

Closed by commit rABCc3f35428fd2e: Add Qt tests for wallet spends (authored by Russell Yanofsky <russ@yanofsky.org>, committed by jasonbcox). · Explain WhyFeb 12 2019, 22:28

This revision was automatically updated to reflect the committed changes.

jasonbcox added a commit: rSTAGINGc3f35428fd2e: Add Qt tests for wallet spends.Sep 18 2019, 22:22