Enable python regression test for Windows #6548

ptschip · 2015-08-11T17:07:01Z

Two files changed:

rpc-test.sh - enable windows regression test and set env variable so that run-bitcoin-cli does
not get invoked as python can not invoke this on a windows machine
util.py - added handling of NUL files because /dev/null is not recognized on windows
Also, although not required added some additional handling of cache files in the event
that one or more are missing as happened while I was debugging.

theuni · 2015-08-11T17:16:05Z

No need to close/open PRs to get them to rebuild, just force-push the changes.

What's the problem with run-bitcoin-cli on windows? Is it just that it's missing an extension? If so, does making it a ".sh" fix the problem? The wrapper is necessary (at least on Linux) to compare output with expected EOLs.

Do these tests pass on a native windows machine?

ptschip · 2015-08-11T17:58:28Z

run-bitcoin-cli does not run on native windows as a subprocess invoked
from within python...hence the workaround to only invoke bitcoin-cli.exe
directly on native windows.

These tests run fine on native windows until it gets to rest.py where it
"asserts". I didn't realize that all the tests had to pass in order to
get this change in.

I think the problem I'm seeing here in the travis build is the script is
getting hung up when running the wallet.py test. I think i'll update my
master and rebuild then try it out again on my machine again.

On 11/08/2015 10:16 AM, Cory Fields wrote:

No need to close/open PRs to get them to rebuild, just force-push the
changes.

What's the problem with run-bitcoin-cli on windows? Is it just that
it's missing an extension? If so, does making it a ".sh" fix the
problem? The wrapper is necessary (at least on Linux) to compare
output with expected EOLs.

Do these tests pass on a native windows machine?

—
Reply to this email directly or view it on GitHub
#6548 (comment).

theuni · 2015-08-11T18:38:15Z

Rather than adding to the quirks here, it'd probably be easier to just move rpc-tests.sh/tests-config.sh/run-bitcoin-cli all to python, then use the vars directly. I think that would untangle this a bit.

ptschip · 2015-08-12T05:36:06Z

I think on a more elegant solution...

but first

after i was able to compile the master-branch, i ran the py test suite
on native windows and it works just fine with the exception of
walletbackup.py (I'll take a look at that tomorrow)..

but the main problem appears to be that the very first python script get
hung up during the Travis build process. Do you know if they are
actually running the tests on native windows, or through some sort of
windows emulation? I suspect the latter and that's probably why they're
getting hung. I'll take a further look.

On 11/08/2015 11:39 AM, Cory Fields wrote:

Rather than adding to the quirks here, it'd probably be easier to just
move rpc-tests.sh/tests-config.sh/run-bitcoin-cli all to python, then
use the vars directly. I think that would untangle this a bit.

—
Reply to this email directly or view it on GitHub
#6548 (comment).

laanwj · 2015-08-12T07:04:29Z

it'd probably be easier to just move rpc-tests.sh/tests-config.sh/run-bitcoin-cli all to python, then use the vars directly. I think that would untangle this a bit.

Migrating the scripts to Python is always a good thing in my book. +1

jonasschnelli · 2015-08-12T13:07:02Z

Moving rpc-tests.sh to Python would definitively make sense. Should also work for the Makefile .in sources.

jonasschnelli · 2015-08-12T13:12:18Z

qa/rpc-tests/test_framework/util.py

+        return open("NUL", "w+")
+    else:
+	return open("/dev/null", "w+")
+


The devnull switch is not required.
I think you can do devnull = open(os.devnull, 'w') at L96 and L188.

i'll add that

On 12/08/2015 6:12 AM, Jonas Schnelli wrote:

In qa/rpc-tests/test_framework/util.py
#6548 (comment):

@@ -20,6 +20,12 @@
from authproxy import AuthServiceProxy, JSONRPCException
from util import *

+def null_file():

if os.name == 'nt':

return open("NUL", "w+")

else:

return open("/dev/null", "w+")

The devnull switch is not required.
I think you can do |devnull = open(os.devnull, 'w')| at L96 and L188.

—
Reply to this email directly or view it on GitHub
https://github.com/bitcoin/bitcoin/pull/6548/files#r36857141.

ptschip · 2015-08-14T23:14:30Z

I had to finally disable the windows tests because the syncing of nodes on windows is even worse during the Travis build. However the tests do run on a native windows machine if they are enabled in rpc-tests.sh

I think this code is ready to be reviewed and merged. I'd prefer to clean up the already existing tangle of shell scripts and move everything to python in a separate pull if you all agree.

maflcko · 2015-08-15T08:51:18Z

Would you mind squashing some of the commits together?

ptschip · 2015-08-15T17:23:09Z

done...

On 15/08/2015 1:51 AM, MarcoFalke wrote:

Would you mind squashing some of the commits together?

—
Reply to this email directly or view it on GitHub
#6548 (comment).

maflcko · 2015-08-16T12:22:00Z

qa/rpc-tests/walletbackup.py

        shutil.rmtree(self.options.tmpdir + "/node2/regtest/blocks")
        shutil.rmtree(self.options.tmpdir + "/node2/regtest/chainstate")

-        # Restore wallets from backup


Minor "whitespace typo"?

ptschip · 2015-08-20T15:50:22Z

I have the code ready which ports everything to python and gets rid of
tests-config.sh and tests-config.sh.in, but I hesitate to include it
with this pull as the changes are a little more substantial and not
really related to just enabling tests for Windows.

On 12/08/2015 12:05 AM, Wladimir J. van der Laan wrote:

it'd probably be easier to just move
rpc-tests.sh/tests-config.sh/run-bitcoin-cli all to python, then
use the vars directly. I think that would untangle this a bit.
Migrating the scripts to Python is always a good thing in my book. +1

—
Reply to this email directly or view it on GitHub
#6548 (comment).

jonasschnelli · 2015-08-20T15:56:13Z

qa/rpc-tests/test_framework/test_framework.py

@@ -130,9 +128,7 @@ def main(self):
            traceback.print_tb(sys.exc_info()[2])

        if not self.options.noshutdown:
-            print("Stopping nodes")


What's the reason of removing this line?

This gets printed out when stop_nodes() is called. No need to print it
out twice.

On 20/08/2015 8:56 AM, Jonas Schnelli wrote:

In qa/rpc-tests/test_framework/test_framework.py
#6548 (comment):

@@ -130,9 +128,7 @@ def main(self):
traceback.print_tb(sys.exc_info()[2])

if not self.options.noshutdown:

print("Stopping nodes")

What's the reason of removing this line?

—
Reply to this email directly or view it on GitHub
https://github.com/bitcoin/bitcoin/pull/6548/files#r37546694.

jonasschnelli · 2015-08-20T16:03:03Z

Tend to NACK.
IMO to many changes to enable rpc tests on windows platform. Allow rpc test on windows would be valuable though.

Maybe removing all unnecessary changes (refactoring, log informations, workaround) and just fix the minimum plus the root cause of #6554 (in the code and not in the tests).
IMO a unit/RPC test should not use a workarounds to prevent from failing (main code should be fixed).

theuni · 2015-08-20T19:51:41Z

Agreed with @jonasschnelli. Don't break the tests, fix the failures.

Fwiw, this is necessary to get the tests passing via Wine in Linux with no futher work-arounds: theuni@5d2f9fc
The fix is debatable, but it points out the issue and works to mitigate it.

theuni · 2015-08-25T16:07:30Z

@ptschip Let's see how Travis does with #6590.

ptschip · 2015-08-26T13:37:45Z

@theuni After #6590 was merged I have taken out the changes to travis.yml and it looks good. All tests passed. Just a reminder #6590 only enables windows tests to run on Wine and allows the Travis builds to pass, but not enable them to be run on Native Windows. The changes in this pull request will still be needed.

theuni · 2015-08-26T16:12:16Z

Hmm, I thought that would've failed on Travis.

After looking more closely, we no longer test any output from bitcoin-cli. So the line-endings and wrapper-script are now moot.

You can drop the if case in rpc-tests.sh and just make it very simple:

export BITCOINCLI=${REAL_BITCOINCLI}
export BITCOIND=${REAL_BITCOIND}

qa/pull-tester/run-bitcoin-cli can be deleted as well.

ptschip · 2015-08-26T16:29:18Z

great, I was wondering why that was still needed ... deleted
run-bitcoin-cli and made updates

On 26/08/2015 9:12 AM, Cory Fields wrote:

Hmm, I thought that would've failed on Travis.

After looking more closely, we no longer test any output from
bitcoin-cli. So the line-endings and wrapper-script are now moot.

You can drop the if case in rpc-tests.sh and just make it very simple:

export BITCOINCLI=${REAL_BITCOINCLI}
export BITCOIND=${REAL_BITCOIND}

qa/pull-tester/run-bitcoin-cli can be deleted as well.

—
Reply to this email directly or view it on GitHub
#6548 (comment).

theuni · 2015-08-26T16:31:03Z

Why were wallet/walletbackup tests disabled?

ptschip · 2015-08-26T16:31:35Z

opps, accidentally commented out a couple of scripts will try it again.

On 26/08/2015 9:12 AM, Cory Fields wrote:

Hmm, I thought that would've failed on Travis.

After looking more closely, we no longer test any output from
bitcoin-cli. So the line-endings and wrapper-script are now moot.

You can drop the if case in rpc-tests.sh and just make it very simple:

export BITCOINCLI=${REAL_BITCOINCLI}
export BITCOIND=${REAL_BITCOIND}

qa/pull-tester/run-bitcoin-cli can be deleted as well.

—
Reply to this email directly or view it on GitHub
#6548 (comment).

theuni · 2015-08-26T17:04:04Z

@ptschip Need to remove the entry for run-bitcoin-cli in EXTRA_DIST from Makefile.am.

ptschip · 2015-08-26T17:10:57Z

I uploaded the changes but all the builds are failing, however, the
rpc-tests appear to be passing...is there something wrong with the
Travis builds? They are all finishing
running their rpc-tests ok, but at the very end the build is exiting
with a 1.

Cleaning up

Tests successful

The command "if [ "$RUN_TESTS" = "true" ]; then
qa/pull-tester/rpc-tests.sh; fi" exited with 0.

cache.2

store build cache

9.92schange detected:

...

changes detected, packing new archive

.

uploading archive

after_script

0.01s$ {:"if [ "$TRAVIS_PULL_REQUEST" != "false" ]; then (echo
"Upload goes here. Something like"=>"scp -r $BASE_OUTDIR server" ||
echo "upload failed"); fi"}

/home/travis/build.sh: line 41: scp -r
/home/travis/build/bitcoin/bitcoin/out server" || echo "upload failed");
fi}: No such file or directory

Done. Your build exited with 1.

On 26/08/2015 9:31 AM, Cory Fields wrote:

Why were wallet/walletbackup tests disabled?

—
Reply to this email directly or view it on GitHub
#6548 (comment).

ptschip · 2015-08-26T17:12:48Z

ah...ok

On 26/08/2015 10:04 AM, Cory Fields wrote:

@ptschip https://github.com/ptschip Need to remove the entry for
run-bitcoin-cli in EXTRA_DIST from Makefile.am.

—
Reply to this email directly or view it on GitHub
#6548 (comment).

ptschip · 2015-08-26T17:41:08Z

all looks good now

On 26/08/2015 10:04 AM, Cory Fields wrote:

@ptschip https://github.com/ptschip Need to remove the entry for
run-bitcoin-cli in EXTRA_DIST from Makefile.am.

—
Reply to this email directly or view it on GitHub
#6548 (comment).

theuni · 2015-08-26T17:58:13Z

ACK after squashing the last 2 commits.

1) Multiplatorm support for devnull 2) Fixed a bug in the handling of cache files 3) Deleted run-bitcoin-cli as no longer needed

ptschip · 2015-08-26T18:25:42Z

Done.

On 26/08/2015 10:58 AM, Cory Fields wrote:

ACK after squashing the last 2 commits.

—
Reply to this email directly or view it on GitHub
#6548 (comment).

theuni · 2015-08-26T18:34:15Z

ACK. Thanks for sticking with it :)

jonasschnelli · 2015-08-26T18:40:42Z

ACK.
Nice!

ptschip · 2015-08-30T17:14:55Z

@laanwj Any idea if/when this can be merged? I have a couple pulls I'd like to submit that depend on this one...(sorry if I'm sounding pushy, I'm new here and I don't know entirely how you operate yet)

060058e Enable python tests for Native Windows (ptschip)

laanwj · 2015-09-01T09:40:15Z

Sorry, missed this and it got delayed a bit. I really need to start filtering github-mail and bitcoin-dev to different mailboxes.

Backport RPC test harness PRs Cherry-picked from the following upstream PRs: - bitcoin/bitcoin#6548 - bitcoin/bitcoin#6804 - Just the coverage backend, not the flag to enable it. - bitcoin/bitcoin#7744 - bitcoin/bitcoin#9832 - Excludes `wallet-hd.py` change (missing bitcoin/bitcoin#8309). Part of #2074.

Backport RPC test harness PRs Cherry-picked from the following upstream PRs: - bitcoin/bitcoin#6548 - bitcoin/bitcoin#6804 - Just the coverage backend, not the flag to enable it for all RPC tests. - bitcoin/bitcoin#7744 - bitcoin/bitcoin#9832 - Excludes `wallet-hd.py` change (missing bitcoin/bitcoin#8309). Part of #2074.

jonasschnelli reviewed Aug 12, 2015
View reviewed changes

ptschip force-pushed the enable_win_py branch from b18e443 to a93578b Compare August 15, 2015 17:03

maflcko reviewed Aug 16, 2015
View reviewed changes

ptschip force-pushed the enable_win_py branch from c630f34 to 0dc0930 Compare August 18, 2015 20:20

laanwj added the Tests label Aug 19, 2015

ptschip force-pushed the enable_win_py branch from 3cbf5eb to 22fd59c Compare August 20, 2015 04:44

jonasschnelli reviewed Aug 20, 2015
View reviewed changes

ptschip force-pushed the enable_win_py branch from 22fd59c to ad9e3e8 Compare August 21, 2015 12:19

theuni mentioned this pull request Aug 25, 2015

Fix stale socket rebinding and re-enable python tests for Windows #6590

Merged

ptschip force-pushed the enable_win_py branch from 7fc3940 to 6b523f2 Compare August 26, 2015 13:08

ptschip force-pushed the enable_win_py branch from 0943ea8 to 9dc15ee Compare August 26, 2015 16:51

Enable python tests for Native Windows

060058e

1) Multiplatorm support for devnull 2) Fixed a bug in the handling of cache files 3) Deleted run-bitcoin-cli as no longer needed

ptschip force-pushed the enable_win_py branch from 05c0b33 to 060058e Compare August 26, 2015 18:02

laanwj merged commit 060058e into bitcoin:master Sep 1, 2015

laanwj added a commit that referenced this pull request Sep 1, 2015

Merge pull request #6548

10c0e52

060058e Enable python tests for Native Windows (ptschip)

ptschip deleted the enable_win_py branch September 1, 2015 21:11

str4d mentioned this pull request Mar 24, 2020

Backport RPC test harness PRs zcash/zcash#4411

Merged

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

Enable python regression test for Windows #6548

Enable python regression test for Windows #6548

Uh oh!

Conversation

ptschip commented Aug 11, 2015

Uh oh!

theuni commented Aug 11, 2015

Uh oh!

ptschip commented Aug 11, 2015

Uh oh!

theuni commented Aug 11, 2015

Uh oh!

ptschip commented Aug 12, 2015

Uh oh!

laanwj commented Aug 12, 2015

Uh oh!

jonasschnelli commented Aug 12, 2015

Uh oh!

jonasschnelli Aug 12, 2015

Choose a reason for hiding this comment

Uh oh!

ptschip Aug 12, 2015

Choose a reason for hiding this comment

Uh oh!

ptschip commented Aug 14, 2015

Uh oh!

maflcko commented Aug 15, 2015

Uh oh!

ptschip commented Aug 15, 2015

Uh oh!

maflcko Aug 16, 2015

Choose a reason for hiding this comment

Uh oh!

ptschip Aug 18, 2015

Choose a reason for hiding this comment

Uh oh!

ptschip commented Aug 20, 2015

Uh oh!

jonasschnelli Aug 20, 2015

Choose a reason for hiding this comment

Uh oh!

ptschip Aug 20, 2015

Choose a reason for hiding this comment

Uh oh!

jonasschnelli commented Aug 20, 2015

Uh oh!

theuni commented Aug 20, 2015

Uh oh!

theuni commented Aug 25, 2015

Uh oh!

ptschip commented Aug 26, 2015

Uh oh!

theuni commented Aug 26, 2015

Uh oh!

ptschip commented Aug 26, 2015

Uh oh!

theuni commented Aug 26, 2015

Uh oh!

ptschip commented Aug 26, 2015

Uh oh!

theuni commented Aug 26, 2015

Uh oh!

ptschip commented Aug 26, 2015

Uh oh!

ptschip commented Aug 26, 2015

Uh oh!

ptschip commented Aug 26, 2015

Uh oh!

theuni commented Aug 26, 2015

Uh oh!

ptschip commented Aug 26, 2015

Uh oh!

theuni commented Aug 26, 2015

Uh oh!

jonasschnelli commented Aug 26, 2015

Uh oh!

ptschip commented Aug 30, 2015

Uh oh!

laanwj commented Sep 1, 2015

Uh oh!