Page MenuHomePhabricator

Add Qt tests for wallet spends

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


Group Reviewers
Restricted Project
rABCc3f35428fd2e: Add Qt tests for wallet spends

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
Completes T512

Test Plan

make check

Diff Detail

rABC Bitcoin ABC
Automatic diff as part of commit; lint not applicable.
Automatic diff as part of commit; unit tests not applicable.

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
jasonbcox added inline comments.Feb 12 2019, 01:21
37 ↗(On Diff #7307)

The difference here is because we do not support Qt4 (see for reference to the introduction to callback.h which I don't believe to be necessary)

Fabien requested changes to this revision.Feb 12 2019, 08:29

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 ?

17 ↗(On Diff #7307)

Not used, can be removed

24 ↗(On Diff #7307)


26 ↗(On Diff #7307)


36 ↗(On Diff #7307)

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 ↗(On Diff #7307)


144 ↗(On Diff #7307)

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 ↗(On Diff #7307)

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.
36 ↗(On Diff #7307)

I see two reasons for pulling this in:

  1. Reduce merge conflicts.
  2. This is a definite improvement over the old ConfirmSend. We can utilize this if we wish.
144 ↗(On Diff #7307)

Interesting. I figured all objects required names, but these are the only two objects with them and both are unused.

jasonbcox updated this revision to Diff 7313.Feb 12 2019, 19:07
jasonbcox marked 2 inline comments as done.

Fixed according to feedback

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.

Fabien accepted this revision.Feb 12 2019, 20:21
This revision is now accepted and ready to land.Feb 12 2019, 20:21
This revision was automatically updated to reflect the committed changes.