[tests] Functional test warnings #10197

pull jnewbery wants to merge 2 commits into bitcoin:master from jnewbery:functional_test_warnings changing 1 files +20 −4

jnewbery commented at 3:12 PM on April 12, 2017: member
Two changes:
1. Issue warnings when running test_runner:
- if there are python files in the directory that are not listed in test_runner (this is downgraded from an error)
- if there is already a bitcoind process running on the system (uses pidof - only works for linux)
- if there is a cache directory
1. delete cache directory every time test_runner is started. Adds a parameter --keepcache to override the default behaviour and keep the cache directory from the previous run.
Rebuilding the cache takes about 21 seconds on my pc. This will be reduced to just under 2 seconds once #10082 has been fully merged.
jnewbery renamed this:
~~Functional test warnings~~
[tests] Functional test warnings
on Apr 12, 2017
fanquake added the label Tests on Apr 12, 2017
jnewbery force-pushed on Apr 13, 2017

in test/functional/test_runner.py:425 in 3fd09cbbe8 outdated

 421 | @@ -407,9 +422,7 @@ def check_script_list(src_dir):
 422 |      python_files = set([t for t in os.listdir(script_dir) if t[-3:] == ".py"])
 423 |      missed_tests = list(python_files - set(map(lambda x: x.split()[0], ALL_SCRIPTS + NON_SCRIPTS)))
 424 |      if len(missed_tests) != 0:
 425 | -        print("The following scripts are not being run:" + str(missed_tests))
 426 | -        print("Check the test lists in test_runner.py")
 427 | -        sys.exit(1)
 428 | +        print("%sWARNING!%s The following scripts are not being run: %s. Check the test lists in test_runner.py." % (BOLD[1], BOLD[0], str(missed_tests)))

MarcoFalke commented at 9:44 PM on April 14, 2017:

I'd prefer to keep this an error, so that a failing travis will quickly tell the author of a new script that it is not being run.

MarcoFalke commented at 2:22 PM on April 17, 2017:

Maybe make this warning an error on travis?

if os.getenv('TRAVIS')=='true':
  # On travis this warning is an error to prevent merging incomplete commits into master
  sys.exit(1)

MarcoFalke commented at 9:45 PM on April 14, 2017: member

utACK 3fd09cb beside the comment
jnewbery commented at 1:09 PM on April 17, 2017: member

@MarcoFalke I've found the error-and-exit behaviour annoying and intrusive since we introduced it. I'm often working on test cases outside a git repository and either want to add a new test case or modify an existing test case and keep a backup in the same directory (my exact setup is that I'm using rsync to copy my git repo into a directory on a VM, and old files don't automatically get deleted when I checkout a new branch and rsync it to the VM). The fact that test_runner fails to run when it detects a new file in the directory is annoying.

Remember that the point of this check is so new test cases don't get added to the repo without being added to test_runner. Only one person needs to notice that the warning is being raised either locally or on Travis for it to have done its job. For that, a warning should be enough.
MarcoFalke commented at 2:02 PM on April 17, 2017: member

@jnewbery Sounds reasonable.

in test/functional/test_runner.py:255 in 3fd09cbbe8 outdated

 250 |  def run_tests(test_list, src_dir, build_dir, exeext, jobs=1, enable_coverage=False, args=[]):
 251 | +    # Warn if bitcoind is already running (unix only)
 252 | +    try:
 253 | +        if subprocess.check_output(["pidof", "bitcoind"]) is not None:
 254 | +            print("%sWARNING!%s There is already a bitcoind process running on this system. Tests may fail unexpectedly due to resource contention!" % (BOLD[1], BOLD[0]))
 255 | +    except:

MarcoFalke commented at 2:19 PM on April 17, 2017:

except (OSError, subprocess.SubprocessError):

should be enough?

[test] add warnings to test_runner c85b080cc7
[tests] Remove cache directory by default when running test_runner 08e51c1c03
jnewbery force-pushed on Apr 17, 2017
jnewbery commented at 2:32 PM on April 17, 2017: member

Addressed @MarcoFalke's nits
MarcoFalke merged this on Apr 17, 2017
MarcoFalke closed this on Apr 17, 2017
MarcoFalke referenced this in commit d86bb075bf on Apr 17, 2017
jnewbery deleted the branch on Apr 17, 2017
Sjors commented at 1:41 PM on January 2, 2018: member

If a MacOS user installs pidof via Homebrew, subprocess.check_output(["pidof", "bitcoind"]) will return b'' which isn't None. This then causes the "There is already a bitcoind process running on this system" to always appear. Not sure if that's worth fixing.
jnewbery commented at 7:45 PM on January 2, 2018: member

Not sure if that's worth fixing.

Certainly! PRs welcome 🙂
PastaPastaPasta referenced this in commit 55944e9c7f on May 31, 2019
PastaPastaPasta referenced this in commit 3464b5d8ea on Jun 10, 2019
PastaPastaPasta referenced this in commit d9927c3b4e on Jun 10, 2019
PastaPastaPasta referenced this in commit 69f95f9d93 on Jun 11, 2019
PastaPastaPasta referenced this in commit d23fa4bb59 on Jun 11, 2019
PastaPastaPasta referenced this in commit f01985ec5e on Jun 12, 2019
PastaPastaPasta referenced this in commit b76b91d821 on Jun 14, 2019
PastaPastaPasta referenced this in commit eb5a391851 on Jun 15, 2019
PastaPastaPasta referenced this in commit 3585119457 on Jun 19, 2019
barrystyle referenced this in commit 8745045852 on Jan 22, 2020
MarcoFalke locked this on Sep 8, 2021

Contributors

Labels

Linked (view graph)

#10082 [tests] Demostration/concept: Add a TestNode class to test_Framework.py