Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

QA: feature_filelock, interface_bitcoin_cli: Use PACKAGE_NAME in messages rather than hardcoding Bitcoin Core #15896

Merged
merged 1 commit into from Apr 26, 2019

Conversation

Projects
None yet
7 participants
@luke-jr
Copy link
Member

commented Apr 25, 2019

No description provided.

QA: feature_filelock, interface_bitcoin_cli: Use PACKAGE_NAME in mess…
…ages rather than hardcoding Bitcoin Core
@practicalswift

This comment has been minimized.

Copy link
Member

commented Apr 25, 2019

utACK fcc443b

@MarcoFalke MarcoFalke merged commit fcc443b into bitcoin:master Apr 26, 2019

0 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build could not complete due to an error
Details
continuous-integration/appveyor/pr AppVeyor build failed
Details

MarcoFalke added a commit that referenced this pull request Apr 26, 2019

Merge #15896: QA: feature_filelock, interface_bitcoin_cli: Use PACKAG…
…E_NAME in messages rather than hardcoding Bitcoin Core

fcc443b QA: feature_filelock, interface_bitcoin_cli: Use PACKAGE_NAME in messages rather than hardcoding Bitcoin Core (Luke Dashjr)

Pull request description:

ACKs for commit fcc443:
  practicalswift:
    utACK fcc443b

Tree-SHA512: f87cfea3cb2ac716a5c9a507141dcba18cb0e3cbe17a4114ed11fa283c3d38551cc245ef68f8816c51538d492991e71019d20a9ca4acd22af4f99e631c04d33e
@promag

This comment has been minimized.

Copy link
Member

commented Apr 26, 2019

utACK fcc443b.

@hebasto

This comment has been minimized.

Copy link
Member

commented Apr 26, 2019

It seems interface_bitcoin_cli.py is broken in AppVeyor now.

@Sjors

This comment has been minimized.

Copy link
Member

commented Apr 26, 2019

Normally ignoring AppVeyor failures isn't a huge issue, but this PR changed test/config.ini.in.

@@ -16,7 +16,7 @@ def run_test(self):
"""Main test logic"""

cli_response = self.nodes[0].cli("-version").send_cli()
assert "Bitcoin Core RPC client version" in cli_response
assert "{} RPC client version".format(self.config['environment']['PACKAGE_NAME']) in cli_response

This comment has been minimized.

Copy link
@Sjors

Sjors Apr 26, 2019

Member

Can this be replaced with an assert_equal test? That makes it easier to debug whether there's a problem with the testsuite or the binary in AppVeyor.
(most likely the problem is with the test, since this PR only touched the test)

This comment has been minimized.

Copy link
@Sjors

Sjors Apr 26, 2019

Member

Note that in other places self is left out, e.g. '/src/bitcoind' + config["environment"]["EXEEXT"]

This comment has been minimized.

Copy link
@MarcoFalke

MarcoFalke Apr 26, 2019

Member

assert_equal doesn't work because the version is suffixed by the commit

$ ./src/bitcoin-cli -version
Bitcoin Core RPC client version v0.18.99.0-b1e013e4fa

This comment has been minimized.

Copy link
@MarcoFalke

MarcoFalke Apr 26, 2019

Member

I guess you could strip the commit, but 🤷‍♂

@@ -6,6 +6,7 @@
# test/functional/test_runner.py and test/util/bitcoin-util-test.py

[environment]
PACKAGE_NAME=@PACKAGE_NAME@

This comment has been minimized.

Copy link
@Sjors

Sjors Apr 26, 2019

Member

You probably need to add something here, though I'm not sure what:
https://github.com/bitcoin/bitcoin/blob/master/.appveyor.yml#L41-L48

This comment has been minimized.

Copy link
@Sjors

Sjors Apr 26, 2019

Member

Update, indeed, see #15903

MarcoFalke added a commit that referenced this pull request Apr 26, 2019

Merge #15903: appveyor: Write @PACKAGE_NAME@ to config
faebd8c appveyor: Write @PACKAGE_NAME@ to config (MarcoFalke)

Pull request description:

  fix tests which are currently failing on appveyor after #15896

ACKs for commit faebd8:
  Sjors:
    utACK faebd8c if AppVeyor blesses it.
  ryanofsky:
    utACK faebd8c. Not following your own "Please provide clear motivation for your patch" advice maybe, but I gather the motivation is to fix tests which are currently failing on appveyor after #15896?

Tree-SHA512: 645cc9f82a4897659bfd41d0c645e21201c43bceb36a073e7fa9fff6d38e8190e7b23e44f77f18ecf3cd1794a9a11b8cabfb33d1a477e7417d839f9451b8253d
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.