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

test: Support -cli tests using external bitcoin-cli #15155

Closed
wants to merge 1 commit into from

Conversation

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

commented Jan 12, 2019

It can be useful to build only bitcoind and still run the full test suite using an external bitcoin-cli instance.

Use case: Automated package testing can have a build-time dependency on the bitcoin-cli package and use it to run tests on a bitcoind-only build.

@fanquake fanquake added the Tests label Jan 12, 2019

"""Checks whether bitcoin-cli was compiled."""
def is_cli_available(self):
"""Checks whether bitcoin-cli is available."""
if os.getenv("BITCOINCLI", "__NOT_SET__") != "__NOT_SET__":

This comment has been minimized.

Copy link
@kristapsk

kristapsk Jan 12, 2019

Contributor

Why not just - if os.getenv("BITCOINCLI") != None:?

This comment has been minimized.

Copy link
@luke-jr

luke-jr Jan 12, 2019

Author Member

Python's documentation says getenv returns a string, but without a default it returns None in CPython. Providing a default avoids the ambiguity/contradiction.

This comment has been minimized.

Copy link
@kristapsk

This comment has been minimized.

Copy link
@luke-jr

luke-jr Jan 12, 2019

Author Member

"key, default and the result are str."

This comment has been minimized.

Copy link
@kristapsk

kristapsk Jan 12, 2019

Contributor

But it actually returns None, which isn't string, if environment variable is not set, both in Python2 and Python3.

$ python
Python 3.6.5 (default, Sep 11 2018, 13:54:50) 
[GCC 4.9.3] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> import os
>>> type(os.getenv('unsetenvironmentvariable'))
<class 'NoneType'>

This comment has been minimized.

Copy link
@luke-jr

luke-jr Jan 13, 2019

Author Member

Not as a return value.

This comment has been minimized.

Copy link
@laanwj

laanwj Jan 14, 2019

Member

I agree with @kristapsk here. The correct way to do this is:

if os.getenv("BITCOINCLI") is not None:

or, if you don't want to support an empty value / regard an empty value as unset

if not os.getenv("BITCOINCLI"):

Special-casing is a default value is, imo, fragile and ugly.

This comment has been minimized.

Copy link
@luke-jr

luke-jr Jan 14, 2019

Author Member

Special-casing the default here is well-defined, not fragile... the alternatives are what is fragile, since they're not guaranteed behaviours [yet?].

This comment has been minimized.

Copy link
@MarcoFalke

MarcoFalke Jan 14, 2019

Member

Returning None or a string is just the pythonic way to say it returns optional<string>

This comment has been minimized.

Copy link
@laanwj

laanwj Jan 16, 2019

Member

Returning None or a string is just the pythonic way to say it returns optional

Exactly.

@laanwj

This comment has been minimized.

Copy link
Member

commented Jan 14, 2019

Concept ACK anyhow

@laanwj

This comment has been minimized.

Copy link
Member

commented Jan 22, 2019

@luke-jr can you please address the comment, it's a trivial change but not going to be merged like this

@laanwj laanwj added the Up for grabs label Feb 12, 2019

@laanwj

This comment has been minimized.

Copy link
Member

commented Feb 12, 2019

Closing this and putting up for grabs, it's desirable functionality but not implemented in an acceptable way imo.

@laanwj laanwj closed this Feb 12, 2019

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.