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.

Sign up for GitHub

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

Jump to bottom

test: re-enable CLI test support by using EncodeDecimal in json.dumps() #17705

Merged
merged 1 commit into from Dec 12, 2019
Merged

test: re-enable CLI test support by using EncodeDecimal in json.dumps() #17705

merged 1 commit into from Dec 12, 2019

Conversation

fanquake
Copy link
Member

@fanquake fanquake commented Dec 9, 2019

As mentioned in #17675 (comment).

@fanquake fanquake added the Tests label Dec 9, 2019
@practicalswift
Copy link
Contributor

Thanks for following up on this!

ACK b6f9e35 assuming Travis is happy too -- diff looks correct :)

maflcko
maflcko approved these changes Dec 9, 2019
View reviewed changes
Copy link
Member

@maflcko maflcko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK b6f9e35 assuming Travis is happy too -- diff looks correct :)

travis doesn't run with --usecli

@practicalswift
Copy link
Contributor

@MarcoFalke I think it does for wallet_multiwallet.py, wallet_createwallet.py, wallet_watchonly.py and rpc_deriveaddresses.py? :)

@maflcko
Copy link
Member

maflcko commented Dec 9, 2019

@MarcoFalke I think it does for wallet_multiwallet.py, wallet_createwallet.py, wallet_watchonly.py and rpc_deriveaddresses.py? :)

Yeah, but none of these files are touched in this pull

@practicalswift
Copy link
Contributor

@MarcoFalke They all use arg_to_cli which is touched by this pull, no? :)

fanquake added a commit that referenced this pull request Dec 12, 2019
@fanquake
Merge #17705: test: re-enable CLI test support by using EncodeDecimal…
3f1966e 
… in json.dumps()

b6f9e35 test: re-enable CLI test support by using EncodeDecimal in json.dumps() (fanquake)

Pull request description:

  As mentioned in #17675 (comment).

ACKs for top commit:
  practicalswift:
    ACK b6f9e35 assuming Travis is happy too -- diff looks correct :)
  MarcoFalke:
    > ACK b6f9e35 assuming Travis is happy too -- diff looks correct :)

Tree-SHA512: 79fa535cc1756c8ee610a3d6a316a1c4f036797d6990a5620e44985393a2e52f78450f8e0021d0a148c08705fd1ba765508464a365f9030ae0d2cacbd7a93e19
@fanquake fanquake merged commit b6f9e35 into bitcoin:master Dec 12, 2019
@fanquake fanquake deleted the test_framework_json_dumps branch December 12, 2019 01:34
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Dec 12, 2019
@fanquake
Merge bitcoin#17705: test: re-enable CLI test support by using Encode…
62e2618 
…Decimal in json.dumps()

b6f9e35 test: re-enable CLI test support by using EncodeDecimal in json.dumps() (fanquake)

Pull request description:

  As mentioned in bitcoin#17675 (comment).

ACKs for top commit:
  practicalswift:
    ACK b6f9e35 assuming Travis is happy too -- diff looks correct :)
  MarcoFalke:
    > ACK b6f9e35 assuming Travis is happy too -- diff looks correct :)

Tree-SHA512: 79fa535cc1756c8ee610a3d6a316a1c4f036797d6990a5620e44985393a2e52f78450f8e0021d0a148c08705fd1ba765508464a365f9030ae0d2cacbd7a93e19
@MarkLTZ MarkLTZ mentioned this pull request Apr 4, 2020
Closed
jasonbcox pushed a commit to Bitcoin-ABC/bitcoin-abc that referenced this pull request Nov 10, 2020
@fanquake @PiRK
test: re-enable CLI test support by using EncodeDecimal in json.dumps()
47048ca 
Summary:
> As mentioned in
> bitcoin/bitcoin#17675 (comment)

This makes more functional tests compatible with option `--usecli`, by fixing the JSON serialization error for `Decimal` objects. This was the most common error preventing tests to run with `--usecli` in D8316.

I found two more tests that are fixed by this change, that were not activated in the original PR: `wallet_address_types.py` and `feature_csv_activation.py`

Before:
```
$ ninja && test/functional/test_runner.py --usecli 2>&1 | cut -f2 -d'|' |     grep -E ' (Passed|Skipped|Failed|) *$' | sort | uniq -c
     97  ✓ Passed
     49  ○ Skipped
```

After:
```
$ ninja && test/functional/test_runner.py --usecli 2>&1 | cut -f2 -d'|' |     grep -E ' (Passed|Skipped|Failed|) *$' | sort | uniq -c
    110  ✓ Passed
     36  ○ Skipped
```

This is a backport of Core [[bitcoin/bitcoin#17705 | PR17705]]
Depends on D8317

Test Plan: `test/functional/test_runner.py --usecli`

Reviewers: #bitcoin_abc, deadalnix

Reviewed By: #bitcoin_abc, deadalnix

Differential Revision: https://reviews.bitcoinabc.org/D8320
sidhujag pushed a commit to syscoin-core/syscoin that referenced this pull request Nov 10, 2020
@fanquake
Merge bitcoin#17705: test: re-enable CLI test support by using Encode…
c2abcf8 
…Decimal in json.dumps()

b6f9e35 test: re-enable CLI test support by using EncodeDecimal in json.dumps() (fanquake)

Pull request description:

  As mentioned in bitcoin#17675 (comment).

ACKs for top commit:
  practicalswift:
    ACK b6f9e35 assuming Travis is happy too -- diff looks correct :)
  MarcoFalke:
    > ACK b6f9e35 assuming Travis is happy too -- diff looks correct :)

Tree-SHA512: 79fa535cc1756c8ee610a3d6a316a1c4f036797d6990a5620e44985393a2e52f78450f8e0021d0a148c08705fd1ba765508464a365f9030ae0d2cacbd7a93e19
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Dec 16, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@maflcko maflcko maflcko approved these changes

Assignees
No one assigned
Labels
Tests
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@fanquake @practicalswift @maflcko