test: Remove duplicate NodeContext hacks #19098

pull ryanofsky wants to merge 1 commits into bitcoin:master from ryanofsky:pr/qtx changing 7 files +58 −38
  1. ryanofsky commented at 9:02 pm on May 28, 2020: member

    Qt tests currently are currently using two NodeContext structs at the same time, one in interfaces::NodeImpl::m_context, and the other in BasicTestingSetup::m_node, and the tests have hacks transferring state between them.

    Fix this by getting rid of the NodeImpl::m_context struct and making it a pointer. This way a common BitcoinApplication object can be used for all qt tests, but they can still have their own testing setups.

    Non-test code is changing but non-test behavior is still the same as before.

    Motivation for this PR is to be able to remove the “std::move(test.m_node.connman)” and mempool hacks for swapping individual NodeContext members in Qt tests, because followup PR #19099 adds yet another member (wallet_client) that needs to be swapped. After this change, the whole NodeContext struct can be swapped instead of individual members, so the workarounds are less fragile and invasive.

  2. DrahtBot added the label GUI on May 28, 2020
  3. DrahtBot added the label Tests on May 28, 2020
  4. in src/qt/test/apptests.cpp:66 in 09569ffe62 outdated 
    63@@ -63,6 +64,7 @@ void AppTests::appTests()
64 #endif
65 
66     BasicTestingSetup test{CBaseChainParams::REGTEST}; // Create a temp data directory to backup the gui settings to
    MarcoFalke commented at 10:33 pm on May 28, 2020:
    Duplicate testing setups are illegal anyway. Have you seen https://github.com/bitcoin/bitcoin/pull/18923/commits/fa5f7fa24cc5ec34f3ba9616e6941123f751d8ed which is a massively smaller diff, potentially fixing the issue you ran into.
    MarcoFalke commented at 11:20 pm on May 28, 2020:
    #18923
    ryanofsky commented at 4:25 am on May 29, 2020:

    re: #19098 (review)

    Duplicate testing setups are illegal anyway. Have you seen fa5f7fa which is a massively smaller diff, potentially fixing the issue you ran into.

    Acked other PR, and this PR overlaps a little bit with another part of it, but this PR is solving a different problem: not duplicate test setups but duplicate NodeContext (and WalletContext) instances inside a single test setup and confusion and fragility created by this. I added more motivation to PR description above.

    Also, the diff isn’t very big here, it’s mostly just replacing . with ->

  5. DrahtBot commented at 6:50 am on May 29, 2020: member

    The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #19619 (Remove wallet.dat path handling from wallet.cpp, rpcwallet.cpp by ryanofsky)
    • #19471 (util: Make default arg values more specific by hebasto)
    • #19425 (refactor: Get rid of more redundant chain methods by ryanofsky)
    • #19160 (multiprocess: Add basic spawn and IPC support by ryanofsky)
    • #19101 (refactor: remove ::vpwallets and related global variables by ryanofsky)
    • #19099 (refactor: Move wallet methods out of chain.h and node.h by ryanofsky)
    • #18077 (net: Add NAT-PMP port forwarding support by hebasto)
    • #15454 (Remove the automatic creation and loading of the default wallet by achow101)
    • #10102 ([experimental] Multiprocess bitcoin by ryanofsky)

    If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

  6. ryanofsky force-pushed on May 29, 2020
  7. ryanofsky commented at 12:07 pm on May 29, 2020: member
    Updated 09569ffe624ff46bb44c6b8d1dac8b3d2193c0d4 -> 8569817a4341220d734d445b5846bb01b57fa303 (pr/qtx.1 -> pr/qtx.2, compare) with fix for address book asan error https://travis-ci.org/github/bitcoin/bitcoin/jobs/692330030#L4304 Updated 8569817a4341220d734d445b5846bb01b57fa303 -> 934a8ae4b43350b5a2852ce7de7c87ace25ecfb5 (pr/qtx.2 -> pr/qtx.3, compare) fixing pedantic asan null reference error https://travis-ci.org/github/bitcoin/bitcoin/jobs/692528901#L4294 Rebased 934a8ae4b43350b5a2852ce7de7c87ace25ecfb5 -> d2c9cd790b05b50292c604d5d4b36d35b58448b7 (pr/qtx.3 -> pr/qtx.4, compare) due to conflict with #19176 Rebased d2c9cd790b05b50292c604d5d4b36d35b58448b7 -> ab1736cd92ee2ce0bdfe3c9bf23594cd2213b659 (pr/qtx.4 -> pr/qtx.5, compare) due to conflict with #19310
  8. MarcoFalke deleted a comment on May 29, 2020
  9. ryanofsky force-pushed on May 29, 2020
  10. tarboss commented at 8:19 am on May 30, 2020: none

    good approach, in wallettests.cpp the function TestGui() uses the codeline: “make CWallet()” to create the CWallet. The chain parameter is from 1. Test (Apptests) in the current master branch, and not from Wallettest setup!!!!.

    I tried this (dont wanna break whole top node interface):

    test.m_node.chain = interfaces::MakeChain(test.m_node); //node.context()->connman = std::move(test.m_node.connman); //node.context()->mempool = std::move(test.m_node.mempool); cwallet = std::make_shared(test.m_node.chain.get(), location L"", WalletDBmock)

    btw: walletlocation is empty, but the wallettest run thru, even if u use QApplication app(argc, argv) & comment all other tests out.

  11. ryanofsky commented at 1:38 pm on June 5, 2020: member

    re: tarboss #19098 (comment)

    i tried this

    It sounds like you tried this and got expected result, but let me know if I’m misunderstanding. Thanks for the feedback

  12. DrahtBot added the label Needs rebase on Jun 9, 2020
  13. ryanofsky force-pushed on Jun 10, 2020
  14. DrahtBot removed the label Needs rebase on Jun 10, 2020
  15. DrahtBot added the label Needs rebase on Jun 18, 2020
  16. ryanofsky force-pushed on Jun 19, 2020
  17. DrahtBot removed the label Needs rebase on Jun 19, 2020
  18. in src/qt/test/apptests.cpp:8 in ab1736cd92 outdated 
    5@@ -6,6 +6,7 @@
6 
7 #include <chainparams.h>
8 #include <key.h>
9+#include <interfaces/node.h>
    promag commented at 10:38 pm on July 6, 2020:
    nit, sort.
    ryanofsky commented at 5:16 pm on July 8, 2020:

    re: #19098 (review)

    nit, sort.

    Good catch, fixed

  19. promag commented at 10:41 pm on July 6, 2020: member

    Code review ACK ab1736cd92ee2ce0bdfe3c9bf23594cd2213b659.

    (don’t mind the nit unless this needs a push)

  20. DrahtBot added the label Needs rebase on Jul 7, 2020
  21. ryanofsky force-pushed on Jul 8, 2020
  22. ryanofsky force-pushed on Jul 8, 2020
  23. ryanofsky force-pushed on Jul 8, 2020
  24. ryanofsky force-pushed on Jul 8, 2020
  25. ryanofsky commented at 5:18 pm on July 8, 2020: member
    Rebased ab1736cd92ee2ce0bdfe3c9bf23594cd2213b659 -> fa522f9b9b3ac78d6f52554ad3f468dcb6e42288 (pr/qtx.5 -> pr/qtx.6, compare) due to conflict with #19219 Updated fa522f9b9b3ac78d6f52554ad3f468dcb6e42288 -> 12135dd28ef7619b1508e98e307c0a66bf7dd4a3 (pr/qtx.6 -> pr/qtx.7, compare) with suggested include fix Rebased 12135dd28ef7619b1508e98e307c0a66bf7dd4a3 -> 2dbdb4355b873223388b3c2a737b1911bc179e43 (pr/qtx.7 -> pr/qtx.8, compare) due to conflict with #18923
  26. DrahtBot removed the label Needs rebase on Jul 8, 2020
  27. DrahtBot added the label Needs rebase on Jul 11, 2020
  28. test: Remove duplicate NodeContext hacks 
    Qt tests currently are currently using two NodeContext structs at the same
time, one in interfaces::NodeImpl::m_context, and the other in
BasicTestingSetup::m_node, and the tests have hacks transferring state between
them.

Fix this by getting rid of the NodeImpl::m_context struct and making it a
pointer. This way a common BitcoinApplication object can be used for all qt
tests, but they can still have their own testing setups.

Non-test code is changing but non-test behavior is still the same as before.

Motivation for this PR is to be able to remove the
"std::move(test.m_node.connman)" and mempool hacks for swapping individual
NodeContext members in Qt tests, because followup PR #19099 adds yet another
member (wallet_client) that needs to be swapped. After this change, the whole
NodeContext struct can be swapped instead of individual members, so the
workarounds are less fragile and invasive.
    edc316020e
  29. ryanofsky force-pushed on Jul 13, 2020
  30. DrahtBot removed the label Needs rebase on Jul 13, 2020
  31. in src/test/util/setup_common.cpp:13 in 2dbdb4355b outdated 
     9@@ -10,6 +10,7 @@
10 #include <consensus/params.h>
11 #include <consensus/validation.h>
12 #include <crypto/sha256.h>
13+#include <interfaces/chain.h>
    promag commented at 10:38 pm on July 16, 2020:
    nit, sort 🙊 🏃
    ryanofsky commented at 3:55 pm on July 21, 2020:

    re: #19098 (review)

    nit, sort

    Thanks, fixed

  32. in src/qt/bitcoin.h:93 in 2dbdb4355b outdated 
    89@@ -90,6 +90,8 @@ class BitcoinApplication: public QApplication
90     /// Setup platform style
91     void setupPlatformStyle();
92 
93+    interfaces::Node& node() { return m_node; }
    promag commented at 10:48 pm on July 16, 2020:
    Add only when needed?
    ryanofsky commented at 3:55 pm on July 21, 2020:

    re: #19098 (review)

    Add only when needed?

    Thanks, removed here. It’s added in https://github.com/bitcoin-core/gui/pull/35

  33. ryanofsky force-pushed on Jul 21, 2020
  34. ryanofsky commented at 3:58 pm on July 21, 2020: member

    Thanks for review! Updated with suggestions

    Updated 2dbdb4355b873223388b3c2a737b1911bc179e43 -> edc316020e8270dafc5e31465d532baebdafd3dd (pr/qtx.8 -> pr/qtx.9, compare)

  35. MarcoFalke commented at 6:39 am on July 31, 2020: member

    crACK edc316020e8270dafc5e31465d532baebdafd3dd 🌮

    Signature:

     0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA512
 2
 3crACK edc316020e8270dafc5e31465d532baebdafd3dd 🌮
 4-----BEGIN PGP SIGNATURE-----
 5
 6iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
 7pUgNVQv/Rp1DWQ63V3hRrPG3/Uu713VE7nYlG/B4T37Jkj0PVR5vKFj0pkT45s9Y
 83yDHFDhP+yTQjH5e+mCATQNs9+60D/1f6776O7CLIkiZ2Mh+K+g6Q807feo+AVTu
 9R7Ra56ZgXADrIvICWu7N7AOElNh/gwA+JHX13DcS4abLo13c+Xa9X5ILAB3L2uV+
10Vv8LxivWpo/yF6pNifVsj6rMuvpNzjvKimYM2gu8zPKuXhRye+KqM6KkuFUgiY2B
11TY/c0MGzhbxNTXhiCnYqfgNRT4s6ZO35W3CzReX72Y6g3LsjQ4sTp+pi67vkkP84
12vo5OhC6XgFvX8/ZS66eyPmaOtkmxP7ehydr16NDosACfnnI7SDTGeSApICTyn00i
13ynEE2kcXL/KY1wKhcGKq0J6mbXuzTPL5GGOJTkxyXeFdJCYe1PEPv0E1wvt0f7vR
14AKeEjSlf7J3Wv4PT+1OdcrFVSZ19YC27z1mQ1q/UMNTZvwqLfMIm7rTcFpKjHESs
153zxlj4ev
16=9d77
17-----END PGP SIGNATURE-----

    Timestamp of file with hash e4953628661a432a6114bf785c7dd10e546b98aee3b7f032fe9b6cd632fd6334 -

  36. MarcoFalke closed this on Jul 31, 2020
  37. MarcoFalke reopened this on Jul 31, 2020
  38. MarcoFalke removed the label GUI on Jul 31, 2020
  39. promag commented at 10:07 pm on August 6, 2020: member
    ACK edc316020e8270dafc5e31465d532baebdafd3dd.
  40. MarcoFalke merged this on Aug 7, 2020
  41. MarcoFalke closed this on Aug 7, 2020
  42. sidhujag referenced this in commit 1e78f38193 on Aug 7, 2020
  43. jasonbcox referenced this in commit c146a4640b on Oct 20, 2020
  44. ryanofsky referenced this in commit 5baa88fd38 on Dec 8, 2020
  45. DrahtBot locked this on Feb 15, 2022


ryanofsky MarcoFalke DrahtBot tarboss promag

Labels
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-12-18 15:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me