test: Adapt test framework for chains other than "regtest"

MarcoFalke commented at 8:04 pm on July 31, 2019: member

This is required for various work in progress:

testchains #8994
signet #16411
some of my locally written tests

While it will be unused in the master branch as of now, it will make all of those pull requests shorter. Thus review for non-regtest tests can focus on the actual changes and not some test framework changes.

MarcoFalke force-pushed on Jul 31, 2019

test: Fix “local variable 'e' is assigned to but never used”

flake8 F841 lints, as of flake8 3.6.0

68f546635d

MarcoFalke force-pushed on Jul 31, 2019

DrahtBot added the label Tests on Jul 31, 2019

test: Adapt test framework for chains other than "regtest"

Co-Authored-By: Jorge Timón <jtimon@jtimon.cc>

fa8a1d7ba3

MarcoFalke force-pushed on Jul 31, 2019

MarcoFalke commented at 9:04 pm on July 31, 2019: member

Force pushed to make linters happy

DrahtBot commented at 9:25 pm on July 31, 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:

#16411 (Signet support by kallewoof)
#16365 (Log RPC parameters (arguments) if -debug=rpcparams by LarryRuane)
#12134 (Build previous releases and run functional tests by Sjors)

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.

jtimon commented at 6:30 pm on August 1, 2019: contributor

Oh, nice. utACK, but it needs rebase.

MarcoFalke commented at 7:17 pm on August 1, 2019: member

Oh, nice. utACK, but it needs rebase.

It is not tagged with “Needs rebase”. Am I missing something?

jtimon commented at 8:51 pm on August 1, 2019: contributor

Mhmm, that’s weird, but never mind, github doesn’t tell me there’s conflicts now. Still concept ACK.

in test/functional/combine_logs.py:86 in fa8f7552d8 outdated

81+    chain = glob.glob("{}/node0/*/debug.log".format(tmp_dir))
82+    if chain:
83+        chain = chain[0]  # pick the first one if more than one chain was found (should never happen)
84+        chain = chain.split('/node0/')[-1].split('/debug.log')[0]  # extract the chain name
85+    else:
86+        chain = 'regtest'  # fallback to regtest (should only happen when none exists)

kallewoof commented at 4:19 am on August 2, 2019:

Maybe

0chain = re.search('node0/(.+?)/debug', chain[0]).group(1) if chain else 'regtest'

MarcoFalke commented at 12:14 pm on August 2, 2019:

I like the multi-line version because it allows me to add comments. Though, I am happy to use the re solution if others think it is more readable/worthwhile.

kallewoof commented at 12:51 pm on August 2, 2019:

You could also just do L84 as

0chain = re.search('node0/(.+?)/debug', chain).group(1)

kallewoof approved

kallewoof commented at 4:21 am on August 2, 2019: member

Obvious concept ACK, utACK

Nice!

test: Avoid hardcoding the chain name in combine_logs faf36838bd

MarcoFalke force-pushed on Aug 2, 2019

MarcoFalke commented at 1:05 pm on August 2, 2019: member

Changed last commit to address feedback by @kallewoof

practicalswift commented at 5:14 pm on August 2, 2019: contributor

Concept ACK: nice work!

jonatack commented at 6:55 pm on August 2, 2019: member

ACK faf36838bdba7393960fce6ad0c56dc1f93f5870, ran tests and reviewed the code.

MarcoFalke merged this on Aug 5, 2019

MarcoFalke closed this on Aug 5, 2019

MarcoFalke referenced this in commit d5ea8f4bf3 on Aug 5, 2019

MarcoFalke deleted the branch on Aug 5, 2019

sidhujag referenced this in commit 511873dd22 on Aug 10, 2019

jtimon commented at 11:34 am on February 4, 2020: contributor

For this to be useful for #8994 it needed to be complete, which is not. And you have been opposing its completion very actively in #16681 using different excuses/reasons. For signet, no old test (ie feature_assumevalid.py, etc) needs to be updated, only the test framework. So can you re-explain your motivation for changing old tests? I really want to understand how you reconcile changing s/“regtest”/self.chain/ for no apparent reason and in some places but not others with opposing to replacing s/“regtest”/self.chain/ in all places under various arguments that could also be applied here.

MarcoFalke referenced this in commit f32564f0a7 on Feb 4, 2020

MarcoFalke commented at 9:06 pm on February 4, 2020: member

So can you re-explain your motivation for changing old tests?

The changes here were required to even be able to set a different chain (e.g. testnet) for new tests. The changes in #16681, are refactoring old tests that don’t need their chain-name changed. I think there have been two oppositions to rename regtest to regtest2. However, the changes in #16681 are (concept) ACKed by a few people and I am the only one who objected them in the past. I will go ahead and just merge them, if other people think they are useful to them. After all those changes alone won’t cause any harm.

sidhujag referenced this in commit e5ba378689 on Feb 9, 2020

deadalnix referenced this in commit b4d97684be on May 3, 2020

sidhujag referenced this in commit 269521a1d5 on Nov 10, 2020

Fabcien referenced this in commit 736e9068fc on Dec 22, 2020

KolbyML referenced this in commit 9dfef518f7 on Jan 17, 2021

KolbyML referenced this in commit ebf48e94dd on Jan 17, 2021

UdjinM6 referenced this in commit 9a5d7fc5ac on Jan 21, 2021

PastaPastaPasta referenced this in commit e197f976e1 on Jan 22, 2021

PastaPastaPasta referenced this in commit 176f780581 on Jan 22, 2021

DrahtBot locked this on Feb 15, 2022

test: Adapt test framework for chains other than “regtest” #16509

Conflicts