Page MenuHomePhabricator

Add Qt tests for wallet spends
ClosedPublic

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

Details

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
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

src/qt/test/wallettests.cpp
37 ↗(On Diff #7307)

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)

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 ?

src/qt/test/wallettests.cpp
17 ↗(On Diff #7307)

Not used, can be removed

24 ↗(On Diff #7307)

dito

26 ↗(On Diff #7307)

dito

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)

braces

src/qt/transactionview.cpp
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 added inline comments.
src/qt/test/wallettests.cpp
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.
src/qt/transactionview.cpp
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 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.

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