Tests: Use self.chain instead of ‘regtest’ in all current tests #16681

pull jtimon wants to merge 1 commits into bitcoin:master from jtimon:b19-testchains-tests changing 16 files +44 −44
  1. jtimon commented at 2:16 pm on August 22, 2019: contributor

    Simply avoiding the hardcoded string in more places for consistency. It can also allow for more easily reusing tests for other chains other than regtest.

    Separated from #8994 . Continues #16509 .

    It is still not complete (ie to be complete, we need the -chain parameter in #16680 and make whether acceptnonstdtxs is allowed for that chain or not customizable for regtest [or for custom chains like in #8994 ] ). But while being incomplete like #16509 , it’s quite simple to review and another step forward IMO.

  2. DrahtBot added the label Tests on Aug 22, 2019
  3. jtimon force-pushed on Aug 22, 2019
  4. jtimon force-pushed on Aug 22, 2019
  5. jtimon renamed this:
    QA: Adapt BitcoinTestFramework for chains other than "regtest"
    Tests: Use self.chain instead of 'regtest' in all current tests
    on Aug 22, 2019
  6. Sjors commented at 6:03 pm on August 22, 2019: member
    Concept ACK
  7. DrahtBot commented at 9:17 pm on August 22, 2019: 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:

    • #16895 (External signer multisig support by Sjors)
    • #16549 ([WIP] UI external signer support (e.g. hardware wallet) by Sjors)
    • #16546 ([WIP] External signer support - Wallet Box edition by Sjors)
    • #16528 (Native Descriptor Wallets using DescriptorScriptPubKeyMan by achow101)

    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.

  8. jtimon force-pushed on Sep 6, 2019
  9. jtimon commented at 9:20 pm on September 6, 2019: contributor
    Now also in newly included wallet_reorgsrestore.py
  10. laanwj commented at 1:56 pm on September 10, 2019: member
    I don’t understand the rationale behind this. How much chance do these tests have of working with a non-regtest chain?
  11. instagibbs commented at 5:59 pm on September 10, 2019: member

    @laanwj with a custom testchain, most tests should work out of the box. Only ones that rely on specific genesis block information will break(there are a couple).

    By itself it isn’t greatly motivated, but it’s also not a burden to maintain, so for me concept ACK.

  12. laanwj commented at 6:42 pm on September 10, 2019: member

    Many of the functional tests depend on specific parameters of the regtest chain to be able to test things: reward schedule, mining difficulty, even address format.

    Anyhow, conecpt ACK on this change, I was just wondering.

  13. jtimon commented at 7:12 pm on September 10, 2019: contributor

    I don’t understand the rationale behind this. How much chance do these tests have of working with a non-regtest chain?

    Well, any custom chain created with #8994 should pass all the tests, like, for example, does -chain=regtest2 (in that PR). Other chains like signets I guess don’t benefit much from this change. Projects based on bitcoin core that periodically rebase like elements should benefit too (although, yes, they will need further changes, this won’t be enough for them).

    I guess separating it is mostly about:

    1. Making #8994 smaller if this get merged first so that it’s easier to review and maintain

    2. hopefully new tests will copy the self.chain from existing tests instead of copying the “regtest”. That would mean less work when rebasing #8994

    In summary, it’s basically about #8994 , so if it doesn’t make sense separated, I understand too.

  14. jtimon force-pushed on Sep 10, 2019
  15. jtimon force-pushed on Sep 12, 2019
  16. laanwj commented at 11:34 am on September 26, 2019: member

    Well I think in any case it’s better to have the chain configurable instead of hardcoding a magic string "regtest".

    ACK 72d822fc2c46d55d959cc8d2f50145d8607e7126

  17. MarcoFalke commented at 4:19 pm on October 2, 2019: member
    +-0 self.chain and regtest are the same for these test and I don’t think it will ever change
  18. jtimon commented at 4:53 pm on October 2, 2019: contributor
    @MarcoFalke does that imply a nack to #8994 ? Because that changes there.
  19. jtimon commented at 4:57 pm on October 2, 2019: contributor
    Also, wouldn’t it make sense alone, if anything, for consistency with #16509 ?
  20. MarcoFalke commented at 5:47 pm on October 2, 2019: member

    does that imply a nack to #8994 ?

    Yeah, I am -0 on changing to regtest2.

    Also, wouldn’t it make sense alone, if anything, for consistency with #16509 ?

    Yeah, +0 on this.

    So overall +-0

  21. Sjors commented at 8:04 am on October 6, 2019: member
    ACK 72d822f. Tests still pass on macOS 10.14.6.
  22. MarcoFalke commented at 5:14 pm on October 7, 2019: member
    Also, this is not a complete fix, since it is impossible to protect against re-introduction of regtest in new code. E.g. https://github.com/bitcoin/bitcoin/blob/eeecdfa27a14df83c441cb4b04c0ae78a2ccbd2c/test/functional/feature_loadblock.py#L39
  23. MarcoFalke commented at 5:14 pm on October 7, 2019: member
    So, I’ll change to overall -0.1
  24. laanwj commented at 8:23 am on October 8, 2019: member

    Also, this is not a complete fix, since it is impossible to protect against re-introduction of regtest in new code. E.g.

    Strictly you could: randomize the test chain name every time. Not sure it’s worth it though …

    Yeah, I am -0 on changing to regtest2.

    I don’t think we should change the default name of the regtest network either. The only chain we’d potentially rename would be testnet3, if the rules change, but an instance of regtest is meant to be short-lived.

  25. jtimon force-pushed on Oct 8, 2019
  26. jtimon commented at 8:15 pm on October 8, 2019: contributor

    Also, this is not a complete fix,…

    Rebased on top of master, but I don’t see that test in master. It seems that commit doesn’t belong in master unless I’m missing something.

    I added https://github.com/jtimon/bitcoin/commit/e889b4ba1fc20fe49cce2f23f0594fd8bf19993d#diff-6651d2553d181295a8570f18069e6b2bR28 for completion when doing the grep. In any case, even if it’s not complete (and it’s not complete because it lacks something like https://github.com/jtimon/bitcoin/commit/0943ce09151003daf4fc353687e6c084416d0b2a#diff-2a344479dbbded9df5d6a4abde2cd48cL44), it makes #16509 more complete.

    I don’t think we should change the default name of the regtest network either.

    I created #17032 and #17037 separated from #8994 that unlike #8994 don’t change the default self.chain to regtest2. Neither of them (https://github.com/bitcoin/bitcoin/pull/17032 and #17037) need this, but I still think it’s worth it for consistency and to complete #16509.

    And to reiterate @laanwj ’s words:

    Well I think in any case it’s better to have the chain configurable instead of hardcoding a magic string “regtest”.

    But if those reasons aren’t enough and there’s little chances of changing the default “regtest” to “regtest2” in the tests, then #8994 is not a justification for this either (by the way, you can go nack it for that reason, I guess). In that case, I guess we should close this.

    Or perhaps we can wait for #17032 or #17037 to be merged first for this to be able to be complete.

    To be honest, what I care the most about personally now is #17037, not #8994 anymore.

  27. MarcoFalke commented at 8:39 pm on October 8, 2019: member

    In that case, I guess we should close this.

    Ok, closing then

  28. MarcoFalke closed this on Oct 8, 2019
  29. jtimon commented at 1:36 pm on October 9, 2019: contributor
    So consistency and completion are not good enough reasons to do things and neither is avoiding hardcoding strings? Ok, noted.
  30. MarcoFalke reopened this on Oct 10, 2019
  31. MarcoFalke commented at 5:12 pm on October 25, 2019: member

    So consistency and completion are not good enough reasons to do things and neither is avoiding hardcoding strings?

    As mentioned previously, this is not complete nor consistent. If you rebase on master, new regtest strings have been introduced. And there is no sane way to prevent that happen in the future, even if this pull was merged.

    Also, we have a lot more stuff hardcoded to regtest in the functional tests, e.g. the bech32 hrp in ADDRESS_BCRT1_UNSPENDABLE…

  32. jtimon commented at 5:30 pm on October 25, 2019: contributor

    As answered before, is (or was) only not complete for one case whuch is contemplated in other prs. The example you poibted out wasn’t ecen part of master.

    Of course, the more we delay this the more new things there will be to correct, for people will continue to copy “regtest” instead of self.chain from other tests.

    I will grep again later, but I really hope this time you’re not talking about other branches again. Feel free to poibt out which cases make this incomplete according to you.

    On Fri, Oct 25, 2019, 19:15 MarcoFalke notifications@github.com wrote:

    So consistency and completion are not good enough reasons to do things and neither is avoiding hardcoding strings?

    As mentioned previously, this is not complete nor consistent. If you rebase on master, new regtest strings have been introduced. And there is no sane way to prevent that happen in the future, even if this pull was merged.

    Also, we have a lot more stuff hardcoded to regtest in the functional tests, e.g. the bech32 hrp in ADDRESS_BCRT1_UNSPENDABLE…

    — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/bitcoin/bitcoin/pull/16681?email_source=notifications&email_token=AAHWGSV6LZ7PC7YALNVC6ULQQMSTVA5CNFSM4IOVRCR2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOECI7JZI#issuecomment-546436325, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAHWGSSCEHRC5ZGZL64DZB3QQMSTVANCNFSM4IOVRCRQ .

  33. Tests: Use self.chain instead of 'regtest' in almost all current tests 1abcecc40c
  34. jtimon force-pushed on Oct 26, 2019
  35. jtimon commented at 11:26 am on October 26, 2019: contributor
    Rebased, adapted new test feature_loadblock.py
  36. Sjors commented at 6:39 pm on October 27, 2019: member

    re-ACK 1abcecc. I think it’s an improvement even if incomplete and if some PR’s might accidentally bring “regtest” back. Subsequent improvements hopefully don’t have to touch 16 files.

    I’d like to break the circular arguments between this and related pull requests. Those are complicated enough to reason about without “but I don’t like this name change in the other PR”; using self.chain instead of "regtest" gives us more optionality.

  37. ryanofsky approved
  38. ryanofsky commented at 3:42 pm on October 30, 2019: member

    Code review ACK 1abcecc40c518a98b7d17880657ec0247abdf125.

    I think one reason a change like this can get stuck in the review process is not stating a motivation or list of practical advantages in the PR description. When a description begins with “Separated from _. continues _. It is incomplete…”, it suggests a rabbit hole few people will want to go down.

    On the other hand, even though our review process is intentionally conservative, I think if a change is safe and has had sufficient code review that we shouldn’t hold neutral or “+0” feedback against it. If as a reviewer you think drawbacks and risks of a change outweigh its benefits, you should actually state that, so we can avoid uncertainty and open PRs sitting in limbo.

  39. ryanofsky approved
  40. ryanofsky commented at 8:04 pm on November 18, 2019: member
    Code review ACK 1abcecc40c518a98b7d17880657ec0247abdf125
  41. jtimon commented at 11:46 am on February 4, 2020: contributor
    I guess if I rebase now there will be more instances, but then I would lose the 2 ACKs. What should I do?
  42. elichai commented at 1:51 pm on February 4, 2020: contributor
    Code review ACK 1abcecc40c518a98b7d17880657ec0247abdf125 I do wonder how many of these tests will fail on testnet (even a custom one with difficulty 1). Anyhow reducing hard coded strings is good IMHO, and I think even though it’s incomplete it will help prevent future hard coded strings just by the fact that a lot of people copy-paste that code.
  43. ryanofsky commented at 4:12 pm on February 4, 2020: member

    I guess if I rebase now there will be more instances, but then I would lose the 2 ACKs. What should I do?

    I’d suggest rebasing, the changes here are straightforward and I’m happy to reack.

  44. MarcoFalke commented at 8:55 pm on February 4, 2020: member
    This has two ACKs, merging now.
  45. MarcoFalke referenced this in commit f32564f0a7 on Feb 4, 2020
  46. MarcoFalke merged this on Feb 4, 2020
  47. MarcoFalke closed this on Feb 4, 2020
  48. fanquake commented at 0:08 am on February 5, 2020: member
    Opened #18068 to track a followup PR.
  49. MarcoFalke referenced this in commit bd5c4c6971 on Feb 5, 2020
  50. sidhujag referenced this in commit e5ba378689 on Feb 9, 2020
  51. sidhujag referenced this in commit 269521a1d5 on Nov 10, 2020
  52. Fabcien referenced this in commit 736e9068fc on Dec 22, 2020
  53. UdjinM6 referenced this in commit 9a5d7fc5ac on Jan 21, 2021
  54. KolbyML referenced this in commit 123ce70c04 on Jan 21, 2021
  55. KolbyML referenced this in commit c76a0de90e on Jan 21, 2021
  56. KolbyML referenced this in commit ef37c2fad8 on Jan 21, 2021
  57. KolbyML referenced this in commit 16b9d32f5b on Jan 21, 2021
  58. PastaPastaPasta referenced this in commit e197f976e1 on Jan 22, 2021
  59. nunumichael referenced this in commit 918eb146c1 on Feb 16, 2021
  60. DrahtBot locked this on Feb 15, 2022


jtimon Sjors DrahtBot laanwj instagibbs MarcoFalke ryanofsky elichai fanquake

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