Two files changed:
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?
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).
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).
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
rpc-tests.sh to Python would definitively make sense. Should also work for the Makefile .in sources.
19@@ -20,6 +20,12 @@
20 from authproxy import AuthServiceProxy, JSONRPCException
21 from util import *
22
23+def null_file():
24+ if os.name == 'nt':
25+ return open("NUL", "w+")
26+ else:
27+ return open("/dev/null", "w+")
28+
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 (review):
@@ -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.
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.
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).
165@@ -154,11 +166,11 @@ def run_test(self):
166 self.stop_three()
167 self.erase_three()
168
169- # Start node2 with no chain
170+ #Start node2 with no chain
171 shutil.rmtree(self.options.tmpdir + "/node2/regtest/blocks")
172 shutil.rmtree(self.options.tmpdir + "/node2/regtest/chainstate")
173
174- # Restore wallets from backup
140@@ -119,6 +141,7 @@ def run_test (self):
141 assert_equal(self.nodes[0].getbalance(), Decimal('19.99900000'))
142
143 # Sendmany 10 BTC
print, imo.
I’m not sure either. I had put all those prints in there to help me debug and find issue 6554. When you run the python scripts they do help you to understand where the problem is happening. I’d like to leave them in for now but I was thinking that we need to update and standardize the logging of messages, maybe in the next pull after this one. I’m working on getting everything into python, maybe we can update the logging then?
On 16/08/2015 5:23 AM, MarcoFalke wrote:
In qa/rpc-tests/wallet.py #6548 (review):
@@ -119,6 +141,7 @@ def run_test (self): assert_equal(self.nodes[0].getbalance(), Decimal(‘19.99900000’))
0 # Sendmany 10 BTCI am not sure which is preferred, but the code should only have either a comment or the |print|, imo.
— Reply to this email directly or view it on GitHub https://github.com/bitcoin/bitcoin/pull/6548/files#r37145277.
224 node.stop()
225+ print "Waiting for node"+str(i) + " to cleanly exit..."
226 bitcoind_processes[i].wait()
227 del bitcoind_processes[i]
228
229+
97+ or not os.path.isdir(os.path.join("cache", "node2"))
98+ or not os.path.isdir(os.path.join("cache", "node3")) ):
99+
100+ #find and delete old cache directories if any exist
101+ for i in range(4):
102+ if os.path.isdir(os.path.join("cache", "node"+str(i))): shutil.rmtree(os.path.join("cache", "node"+str(i)))
if?
The purpose is to delete any old directories “if they exist” before
creating the cache files anew. For instance, when I found this issue I
was just beginning to debug the code and eventually
found that for some reason only the cache for node0 had been created.
The old code would assume that if the cache for node0 was found then
everything was OK
however the scripts woul fail later on. With the nested loop if a
cache dir is found then it will be deleted so that in the next step all
four can be recreated
to ensure that the cache is complete.
On 16/08/2015 5:24 AM, MarcoFalke wrote:
In qa/rpc-tests/test_framework/util.py #6548 (review):
@@ -78,8 +90,16 @@ def initialize_chain(test_dir): bitcoind and bitcoin-cli must be in search path. """
- if not os.path.isdir(os.path.join(“cache”, “node0”)):
devnull = open("/dev/null", "w+")- if ( not os.path.isdir(os.path.join(“cache”, “node0”))
or not os.path.isdir(os.path.join("cache", "node1"))or not os.path.isdir(os.path.join("cache", "node2"))or not os.path.isdir(os.path.join("cache", "node3")) ):
#find and delete old cache directories if any existfor i in range(4):if os.path.isdir(os.path.join("cache", "node"+str(i))): shutil.rmtree(os.path.join("cache", "node"+str(i)))What is the reason to nest the for loop inside the |if|?
— Reply to this email directly or view it on GitHub https://github.com/bitcoin/bitcoin/pull/6548/files#r37145281.
yes
On 16/08/2015 12:44 PM, MarcoFalke wrote:
In qa/rpc-tests/test_framework/util.py #6548 (review):
@@ -78,8 +90,16 @@ def initialize_chain(test_dir): bitcoind and bitcoin-cli must be in search path. """
- if not os.path.isdir(os.path.join(“cache”, “node0”)):
devnull = open("/dev/null", "w+")- if ( not os.path.isdir(os.path.join(“cache”, “node0”))
or not os.path.isdir(os.path.join("cache", "node1"))or not os.path.isdir(os.path.join("cache", "node2"))or not os.path.isdir(os.path.join("cache", "node3")) ):
#find and delete old cache directories if any existfor i in range(4):if os.path.isdir(os.path.join("cache", "node"+str(i))): shutil.rmtree(os.path.join("cache", "node"+str(i)))So you only want to delete existing cache dirs when at least one cache dir needs to be created?
— Reply to this email directly or view it on GitHub https://github.com/bitcoin/bitcoin/pull/6548/files#r37149332.
Made the code updates you requested and pushed to GitHub…
On 16/08/2015 12:44 PM, MarcoFalke wrote:
In qa/rpc-tests/test_framework/util.py #6548 (review):
@@ -78,8 +90,16 @@ def initialize_chain(test_dir): bitcoind and bitcoin-cli must be in search path. """
- if not os.path.isdir(os.path.join(“cache”, “node0”)):
devnull = open("/dev/null", "w+")- if ( not os.path.isdir(os.path.join(“cache”, “node0”))
or not os.path.isdir(os.path.join("cache", "node1"))or not os.path.isdir(os.path.join("cache", "node2"))or not os.path.isdir(os.path.join("cache", "node3")) ):
#find and delete old cache directories if any existfor i in range(4):if os.path.isdir(os.path.join("cache", "node"+str(i))): shutil.rmtree(os.path.join("cache", "node"+str(i)))So you only want to delete existing cache dirs when at least one cache dir needs to be created?
— Reply to this email directly or view it on GitHub https://github.com/bitcoin/bitcoin/pull/6548/files#r37149332.
42+ j+=1
43 counts = [ x.getblockcount() for x in rpc_connections ]
44+ print " Syncing...Node Block Counts ==> " + str(counts) + ' Elapsed Time (s): ' + str(j)
45 if counts == [ counts[0] ]*len(counts):
46 break
47+ elif j>=120:
elif, also whitespace can be normalized: if t >= 120
ACK
On 16/08/2015 5:24 AM, MarcoFalke wrote:
In qa/rpc-tests/test_framework/util.py #6548 (review):
0 if counts == [ counts[0] ]*len(counts): 1 break
elif j>=120:no need for |elif|, also whitespace can be normalized: |if t >= 120|
— Reply to this email directly or view it on GitHub https://github.com/bitcoin/bitcoin/pull/6548/files#r37145284.
36@@ -36,10 +37,15 @@ def sync_blocks(rpc_connections, wait=1):
37 """
38 Wait until everybody has the same block count
39 """
40+ j=0
41 while True:
42+ j+=1
43 counts = [ x.getblockcount() for x in rpc_connections ]
44+ print " Syncing...Node Block Counts ==> " + str(counts) + ' Elapsed Time (s): ' + str(j)
print "sync_blocks: " + str(counts ...?
The ident was intentional. When you run the script it makes it easier to read the output lines and see what part of the script is running. But as I said, I think the whole logging of messages needs to be updated and standardized.
On 16/08/2015 5:24 AM, MarcoFalke wrote:
In qa/rpc-tests/test_framework/util.py #6548 (review):
0 counts = [ x.getblockcount() for x in rpc_connections ]
print " Syncing...Node Block Counts ==> " + str(counts) + ' Elapsed Time (s): ' + str(j)Should probably say something like |print “sync_blocks: " + str(counts …|?
— Reply to this email directly or view it on GitHub https://github.com/bitcoin/bitcoin/pull/6548/files#r37145288.
36@@ -36,10 +37,15 @@ def sync_blocks(rpc_connections, wait=1):
37 """
38 Wait until everybody has the same block count
39 """
40+ j=0
41 while True:
42+ j+=1
t += wait and move it down to time.sleep(wait)?
ACK
On 16/08/2015 5:25 AM, MarcoFalke wrote:
In qa/rpc-tests/test_framework/util.py #6548 (review):
0 while True:
j+=1Refactor this to |t += wait| and move it down to |time.sleep(wait)|?
— Reply to this email directly or view it on GitHub https://github.com/bitcoin/bitcoin/pull/6548/files#r37145291.
18+ exit
19+else
20+ export BITCOINCLI=${BUILDDIR}/qa/pull-tester/run-bitcoin-cli
21 fi
22
23+export BITCOIND=${REAL_BITCOIND}
no reason, just an oversight, i’ll take it out.
On 16/08/2015 5:25 AM, MarcoFalke wrote:
In qa/pull-tester/rpc-tests.sh #6548 (review):
fi
+export BITCOIND=${REAL_BITCOIND}
Is there a reason to duplicate this from line 9?
— Reply to this email directly or view it on GitHub https://github.com/bitcoin/bitcoin/pull/6548/files#r37145294.
148+ self.sync_all()
149+ self.nodes[3].generate(26)
150+ self.sync_all()
151+ else:
152+ self.nodes[3].generate(101)
153+ self.sync_all()
i hear you, but there’s no other way around for the time being until issue 6554 is fixed. Once fixed we can take out this section of code.
On 17/08/2015 8:30 AM, Jonas Schnelli wrote:
In qa/rpc-tests/walletbackup.py #6548 (review):
self.sync_all()print "Generate 101 more blocks, so any fees paid mature"#separating the block creation in this step is to get around issue [#6554](/bitcoin-bitcoin/6554/) which happens on Windowsif os.getenv('EXEEXT', "") == '.exe':print "WARNING: mining smaller numbers of blocks in this step to workaround issue [#6554](/bitcoin-bitcoin/6554/)"self.nodes[3].generate(25)self.sync_all()self.nodes[3].generate(25)self.sync_all()self.nodes[3].generate(25)self.sync_all()self.nodes[3].generate(26)self.sync_all()else:self.nodes[3].generate(101)self.sync_all()This change feels wrong. Platform specific code in tests and Github issue references in the source code should be avoided. Is there no way to fix the root cause of this issue? If we can’t sync
25 blocks on windows then there is a issue which needs fixing.
— Reply to this email directly or view it on GitHub https://github.com/bitcoin/bitcoin/pull/6548/files#r37199876.
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:
0it'd probably be easier to just move 1rpc-tests.sh/tests-config.sh/run-bitcoin-cli all to python, then 2use 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).
127@@ -130,9 +128,7 @@ def main(self):
128 traceback.print_tb(sys.exc_info()[2])
129
130 if not self.options.noshutdown:
131- print("Stopping nodes")
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 (review):
@@ -130,9 +128,7 @@ def main(self): traceback.print_tb(sys.exc_info()[2])
0 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.
40 while True:
41 counts = [ x.getblockcount() for x in rpc_connections ]
42+ print " Syncing...Node Block Counts ==> " + str(counts) + ' Elapsed Time (s): ' + str(t)
43 if counts == [ counts[0] ]*len(counts):
44 break
45+ if t >= 120:
well, good question..i was just working on the assumption that nodes should sync within a few seconds and 120 seconds was far enough out that it would be obvious that there was a problem with syncing but I don’t know what the requirement really is for how long a node should take to sync…is there such a requirement?
On 20/08/2015 8:57 AM, Jonas Schnelli wrote:
In qa/rpc-tests/test_framework/util.py #6548 (review):
0 if counts == [ counts[0] ]*len(counts): 1 break
if t >= 120:Why 120?
— Reply to this email directly or view it on GitHub https://github.com/bitcoin/bitcoin/pull/6548/files#r37546795.
116@@ -102,7 +117,7 @@ def run_test (self):
117 assert_equal(self.nodes[2].getbalance(), 100)
118 assert_equal(self.nodes[2].getbalance("from1"), 100-21)
119
120- # Send 10 BTC normal
121+ print "Send 10 BTC normal"
wallet.py?
This was/is to help those (me) observe more easily where the sync process gets hung when running on windows.
On 20/08/2015 8:59 AM, Jonas Schnelli wrote:
In qa/rpc-tests/wallet.py #6548 (review):
@@ -102,7 +117,7 @@ def run_test (self): assert_equal(self.nodes[2].getbalance(), 100) assert_equal(self.nodes[2].getbalance(“from1”), 100-21)
# Send 10 BTC normalprint "Send 10 BTC normal"What’s the reason of adding verbose test output to |wallet.py|?
— Reply to this email directly or view it on GitHub https://github.com/bitcoin/bitcoin/pull/6548/files#r37547012.
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).
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: https://github.com/theuni/bitcoin/commit/5d2f9fc158a85f7ed0d72ff46b9344df5304ca25 The fix is debatable, but it points out the issue and works to mitigate it.
Fair enough, I’ve reverted back to just the minimum to get the tests working on Windows and pushed to GitHub for review. I left in one information message so that users can see where the nodes are as far as syncing and there was also one bug fix for the handling of the cache files which I left in.
On 20/08/2015 9:03 AM, Jonas Schnelli wrote:
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 #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).
— Reply to this email directly or view it on GitHub #6548 (comment).
40 while True:
41 counts = [ x.getblockcount() for x in rpc_connections ]
42+ print " Syncing...Node Block Counts ==> " + str(counts) + ' Elapsed Time (s): ' + str(t)
43 if counts == [ counts[0] ]*len(counts):
44 break
45+ t += wait
Actually it’s pretty useful to see where the scripts hang when/if the nodes don’t sync…otherwise the test just hangs there and then proceeds 10 or 15 minutes later without the user ever knowing there was a problem.
On 21/08/2015 6:27 AM, Jonas Schnelli wrote:
In qa/rpc-tests/test_framework/util.py #6548 (review):
0 if counts == [ counts[0] ]*len(counts): 1 break
t += waitMind removing the “syncing…” log prints? Seems that this has nothing to do with running on window?
— Reply to this email directly or view it on GitHub https://github.com/bitcoin/bitcoin/pull/6548/files#r37633247.
OK Jonas, I’ve taken out the log print leaving it as it was.
On 21/08/2015 6:27 AM, Jonas Schnelli wrote:
In qa/rpc-tests/test_framework/util.py #6548 (review):
0 if counts == [ counts[0] ]*len(counts): 1 break
t += waitMind removing the “syncing…” log prints? Seems that this has nothing to do with running on window?
— Reply to this email directly or view it on GitHub https://github.com/bitcoin/bitcoin/pull/6548/files#r37633247.
13- exit 0
14+ export BITCOINCLI=${REAL_BITCOINCLI}
15+ export EXEEXT=${EXEEXT}
16+ echo 'Windows tests are disabled by default due to issue with Travis build'
17+ echo 'To enable Widows tests, edit rpc-tests.sh and comment out "exit"'
18+ exit
s/To enable Widows/To enable Windows/
Why the exit? At least we could disable the RPC test over the travis.yml file for the Win builds (IIRC RPC tests do not run for the current win builds)
Jonas,
good point, I made the changes to travis.yml to disable the tests for the windows builds only , took out the exit , pushed and the build completed successfully
On 22/08/2015 12:27 PM, Jonas Schnelli wrote:
In qa/pull-tester/rpc-tests.sh #6548 (review):
export BITCOIND=${REAL_BITCOIND}
if [ “x${EXEEXT}” = “x.exe” ]; then
- echo “Win tests currently disabled”
- exit 0
- export BITCOINCLI=${REAL_BITCOINCLI}
- export EXEEXT=${EXEEXT}
- echo ‘Windows tests are disabled by default due to issue with Travis build’
- echo ‘To enable Widows tests, edit rpc-tests.sh and comment out “exit”’
- exit
s/To enable Widows/To enable Wi_n_dows/
Why the exit? At least we could disable the RPC test over the |travis.yml| file for the Win builds (IIRC RPC tests do not run for the current win builds)
— Reply to this email directly or view it on GitHub https://github.com/bitcoin/bitcoin/pull/6548/files#r37698868.
29@@ -30,11 +30,11 @@ matrix:
30 - compiler: ": ARM"
31 env: HOST=arm-linux-gnueabihf PACKAGES="g++-arm-linux-gnueabihf" DEP_OPTS="NO_QT=1" GOAL="install" BITCOIN_CONFIG="--enable-glibc-back-compat --enable-reduce-exports"
32 - compiler: ": Win32"
33- env: HOST=i686-w64-mingw32 PACKAGES="nsis gcc-mingw-w64-i686 g++-mingw-w64-i686 binutils-mingw-w64-i686 mingw-w64-dev wine bc" RUN_TESTS=true GOAL="deploy" BITCOIN_CONFIG="--enable-gui --enable-reduce-exports" MAKEJOBS="-j2"
34+ env: HOST=i686-w64-mingw32 PACKAGES="nsis gcc-mingw-w64-i686 g++-mingw-w64-i686 binutils-mingw-w64-i686 mingw-w64-dev wine bc" RUN_TESTS=false GOAL="deploy" BITCOIN_CONFIG="--enable-gui --enable-reduce-exports" MAKEJOBS="-j2"
It’s half-correct. If RUN_TESTS is set to true, not only the unit tests also the rpc test get tested. Because this PR introduces Win32 compatibility of the rpc tests, and they don’t run properly on travis/Win32, they need to be disabled. But only the RPC tests (so maybe a new flag in travis.yml for RUN_RPC_TESTS).
But much better it would be, if the RPC test would succeed on Win32/Travis. I’m pretty convinced that there must be a bug somewhere if the RPC tests do not succeed on Win32.
Wladimir,
The underlying problem there is that due to issue #6554 the Travis build will timeout before the Windows tests can finish…(the nodes will not sync and will hang for anywhere between 15 to 20 minutes before the tests will continue). We can either disable the running of the windows tests in Travis or we can add the code back into the rpc-tests.sh to disable the windows tests by default but then the end users will have to edit rpc-tests.sh to re-enable to run the windows tests. The first method is not ideal but the second is a bit messy.
On 25/08/2015 3:26 AM, Wladimir J. van der Laan wrote:
In .travis.yml #6548 (review):
@@ -30,11 +30,11 @@ matrix: - compiler: “: ARM” env: HOST=arm-linux-gnueabihf PACKAGES=“g++-arm-linux-gnueabihf” DEP_OPTS=“NO_QT=1” GOAL=“install” BITCOIN_CONFIG="–enable-glibc-back-compat –enable-reduce-exports" - compiler: “: Win32”
env: HOST=i686-w64-mingw32 PACKAGES="nsis gcc-mingw-w64-i686 g++-mingw-w64-i686 binutils-mingw-w64-i686 mingw-w64-dev wine bc" RUN_TESTS=true GOAL="deploy" BITCOIN_CONFIG="--enable-gui --enable-reduce-exports" MAKEJOBS="-j2"env: HOST=i686-w64-mingw32 PACKAGES="nsis gcc-mingw-w64-i686 g++-mingw-w64-i686 binutils-mingw-w64-i686 mingw-w64-dev wine bc" RUN_TESTS=false GOAL="deploy" BITCOIN_CONFIG="--enable-gui --enable-reduce-exports" MAKEJOBS="-j2"This is changing RUN_TESTS from true to false. Isn’t that the wrong way around?
— Reply to this email directly or view it on GitHub https://github.com/bitcoin/bitcoin/pull/6548/files#r37851603.
make check should still run.
Oh i see, so it also disables the Unit Tests? That’s not good.
What about if I just add an option to rpc-tests.sh, so that we can keep the Windows regression tests diabled by default so they pass the Travis build, but can then enable them by the end user by passing a “-win” parameter? That gets around the problem of the Travis build failing and also means the end users doesn’t have to edit the shell script to get the tests to work…
On 25/08/2015 4:19 AM, Jonas Schnelli wrote:
In .travis.yml #6548 (review):
@@ -30,11 +30,11 @@ matrix: - compiler: “: ARM” env: HOST=arm-linux-gnueabihf PACKAGES=“g++-arm-linux-gnueabihf” DEP_OPTS=“NO_QT=1” GOAL=“install” BITCOIN_CONFIG="–enable-glibc-back-compat –enable-reduce-exports" - compiler: “: Win32”
env: HOST=i686-w64-mingw32 PACKAGES="nsis gcc-mingw-w64-i686 g++-mingw-w64-i686 binutils-mingw-w64-i686 mingw-w64-dev wine bc" RUN_TESTS=true GOAL="deploy" BITCOIN_CONFIG="--enable-gui --enable-reduce-exports" MAKEJOBS="-j2"env: HOST=i686-w64-mingw32 PACKAGES="nsis gcc-mingw-w64-i686 g++-mingw-w64-i686 binutils-mingw-w64-i686 mingw-w64-dev wine bc" RUN_TESTS=false GOAL="deploy" BITCOIN_CONFIG="--enable-gui --enable-reduce-exports" MAKEJOBS="-j2"It’s half-correct. If |RUN_TESTS| is set to true, not only the unit tests also the rpc test get tested. Because this PR introduces Win32 compatibility of the rpc tests, and they don’t run properly on travis/Win32, they need to be disabled. But /only/ the RPC tests (so maybe a new flag in |travis.yml| for RUN_RPC_TESTS).
But much better it would be, if the RPC test would succeed on Win32/Travis. I’m pretty convinced that there must be a bug somewhere if the RPC tests do not succeed on Win32.
— Reply to this email directly or view it on GitHub https://github.com/bitcoin/bitcoin/pull/6548/files#r37854180.
RUN_RPC_TESTS to travis.yml and distinct between RPC and Unit Test would be an easy solution.
Jonas,
I added RUN_RPC_TESTS to travis.yml…that addition worked quite well.
On 25/08/2015 6:15 AM, Jonas Schnelli wrote:
In .travis.yml #6548 (review):
@@ -30,11 +30,11 @@ matrix: - compiler: “: ARM” env: HOST=arm-linux-gnueabihf PACKAGES=“g++-arm-linux-gnueabihf” DEP_OPTS=“NO_QT=1” GOAL=“install” BITCOIN_CONFIG="–enable-glibc-back-compat –enable-reduce-exports" - compiler: “: Win32”
env: HOST=i686-w64-mingw32 PACKAGES="nsis gcc-mingw-w64-i686 g++-mingw-w64-i686 binutils-mingw-w64-i686 mingw-w64-dev wine bc" RUN_TESTS=true GOAL="deploy" BITCOIN_CONFIG="--enable-gui --enable-reduce-exports" MAKEJOBS="-j2"env: HOST=i686-w64-mingw32 PACKAGES="nsis gcc-mingw-w64-i686 g++-mingw-w64-i686 binutils-mingw-w64-i686 mingw-w64-dev wine bc" RUN_TESTS=false GOAL="deploy" BITCOIN_CONFIG="--enable-gui --enable-reduce-exports" MAKEJOBS="-j2"Haven’t looked at it in detail, but i think adding a variable |RUN_RPC_TESTS| to |travis.yml| and distinct between RPC and Unit Test would be an easy solution.
— Reply to this email directly or view it on GitHub https://github.com/bitcoin/bitcoin/pull/6548/files#r37862781.
Are you saying you have a fix for issue 6554?
The problem we have is not that the tests don’t pass but that they don’t pass before the Travis build times out. When the nodes don’t sync, it can take up to 15 minutes or longer before the tests can resume, on Windows, that is.
On 25/08/2015 8:00 AM, Cory Fields wrote:
@ptschip https://github.com/ptschip Please see my comment above. That change should make the whole Travis discussion moot. In my testing, that allows the tests to pass in Linux with no issues. I’d rather take that (or some variation of it) so that the tests pass, than argue about different ways to disable the tests.
— Reply to this email directly or view it on GitHub #6548 (comment).
Travis doesn’t run the Windows tests on Windows. It runs them with Wine in Linux.
When running the Windows tests with Wine in Linux on my machine, the tests time-out. With the fix above, they work as expected.
I can’t vouch for native Windows, but I would expect Travis to be happy after that change.
I think it’ll be easier if I just open a separate PR, since these are somewhat different issues. Once Travis is running the Windows tests successfully, it’ll be easier to fix them up for native Windows anyway.
Coming right up.
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:
0export BITCOINCLI=${REAL_BITCOINCLI}
1export BITCOIND=${REAL_BITCOIND}
qa/pull-tester/run-bitcoin-cli can be deleted as well.
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).
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).
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).
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).
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).
1) Multiplatorm support for devnull
2) Fixed a bug in the handling of cache files
3) Deleted run-bitcoin-cli as no longer needed
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).
Labels
Tests