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

tests: Enable tests which are incorrectly skipped when running test_runner.py --usecli #17675

Merged
merged 2 commits into from
Dec 9, 2019

Conversation

practicalswift
Copy link
Contributor

@practicalswift practicalswift commented Dec 5, 2019

Annotate functional tests supporting bitcoin-cli (--usecli) as such.

Prior to this commit 74 tests were unnecessarily skipped when running test_runner.py --usecli.

Before:

$ test/functional/test_runner.py --usecli > /dev/null 2>&1
$ echo $?
0
$ test/functional/test_runner.py --usecli 2>&1 | cut -f2 -d'|' | \
    grep -E ' (Passed|Skipped) *$' | sort | uniq -c
      9  ✓ Passed
    126  ○ Skipped

After:

$ test/functional/test_runner.py --usecli > /dev/null 2>&1
$ echo $?
0
$ test/functional/test_runner.py --usecli 2>&1 | cut -f2 -d'|' | \
    grep -E ' (Passed|Skipped) *$' | sort | uniq -c
     83  ✓ Passed
     52  ○ Skipped

Context: --usecli was introduced in f6ade9c

@fanquake fanquake added the Tests label Dec 5, 2019
@DrahtBot
Copy link
Contributor

DrahtBot commented Dec 5, 2019

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #17631 (Expose block filters over REST. by TheBlueMatt)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

@laanwj
Copy link
Member

laanwj commented Dec 6, 2019

Why do some tests not support --cli?
(and wouldn't it be less hassle to mark those? with the idea of fixing them later, maybe)

@practicalswift
Copy link
Contributor Author

Why do some tests not support --cli?

The reasons varies but the most common exception raised is TypeError: Object of type 'Decimal' is not JSON serializable. There is also OSError: [Errno 7] Argument list too long: 'bitcoin-cli' and a few others.

(and wouldn't it be less hassle to mark those? with the idea of fixing them later, maybe)

Good point! Now updated: that brought down the net diff from +80 to +45 (+50-5).

Please re-review :)

@practicalswift practicalswift changed the title tests: Annotate functional tests supporting bitcoin-cli (--usecli) as such tests: Enable tests which are incorrectly skipped when running test_runner.py --usecli Dec 6, 2019
@fanquake
Copy link
Member

fanquake commented Dec 6, 2019

You could also update rpc_deriveaddresses.py which has self.supports_cli = 1.

@practicalswift
Copy link
Contributor Author

@fanquake Good catch. Now updated. Please re-review :)

@laanwj
Copy link
Member

laanwj commented Dec 9, 2019

Code review ACK 5ac804a

TypeError: Object of type 'Decimal' is not JSON serializable

That one should be pretty easy to fix by adding default=EncodeDecimal to json.dumps as done in functional/test_framework/authproxy.py

(no need to do so here, though)

@fanquake fanquake requested a review from maflcko December 9, 2019 14:26
maflcko pushed a commit that referenced this pull request Dec 9, 2019
…running test_runner.py --usecli

5ac804a tests: Use a default of supports_cli=True (instead of supports_cli=False) (practicalswift)
993e38a tests: Mark functional tests not supporting bitcoin-cli (--usecli) as such (practicalswift)

Pull request description:

  Annotate functional tests supporting `bitcoin-cli` (`--usecli`) as such.

  Prior to this commit 74 tests were unnecessarily skipped when running `test_runner.py --usecli`.

  Before:

  ```
  $ test/functional/test_runner.py --usecli > /dev/null 2>&1
  $ echo $?
  0
  $ test/functional/test_runner.py --usecli 2>&1 | cut -f2 -d'|' | \
      grep -E ' (Passed|Skipped) *$' | sort | uniq -c
        9  ✓ Passed
      126  ○ Skipped
  ```

  After:

  ```
  $ test/functional/test_runner.py --usecli > /dev/null 2>&1
  $ echo $?
  0
  $ test/functional/test_runner.py --usecli 2>&1 | cut -f2 -d'|' | \
      grep -E ' (Passed|Skipped) *$' | sort | uniq -c
       83  ✓ Passed
       52  ○ Skipped
  ```

  Context: `--usecli` was introduced in f6ade9c

ACKs for top commit:
  laanwj:
    Code review ACK 5ac804a

Tree-SHA512: 249c0b691a74cf201c729df86c3db2b3faefa53b94703941e566943d252c6d14924e935a8da4f592951574235923fbb7cd22612a5e7e02ff6c762c55a2320ca3
@maflcko maflcko merged commit 5ac804a into bitcoin:master Dec 9, 2019
fanquake added a commit to fanquake/bitcoin that referenced this pull request Dec 9, 2019
fanquake added a commit to fanquake/bitcoin that referenced this pull request Dec 9, 2019
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Dec 9, 2019
…d when running test_runner.py --usecli

5ac804a tests: Use a default of supports_cli=True (instead of supports_cli=False) (practicalswift)
993e38a tests: Mark functional tests not supporting bitcoin-cli (--usecli) as such (practicalswift)

Pull request description:

  Annotate functional tests supporting `bitcoin-cli` (`--usecli`) as such.

  Prior to this commit 74 tests were unnecessarily skipped when running `test_runner.py --usecli`.

  Before:

  ```
  $ test/functional/test_runner.py --usecli > /dev/null 2>&1
  $ echo $?
  0
  $ test/functional/test_runner.py --usecli 2>&1 | cut -f2 -d'|' | \
      grep -E ' (Passed|Skipped) *$' | sort | uniq -c
        9  ✓ Passed
      126  ○ Skipped
  ```

  After:

  ```
  $ test/functional/test_runner.py --usecli > /dev/null 2>&1
  $ echo $?
  0
  $ test/functional/test_runner.py --usecli 2>&1 | cut -f2 -d'|' | \
      grep -E ' (Passed|Skipped) *$' | sort | uniq -c
       83  ✓ Passed
       52  ○ Skipped
  ```

  Context: `--usecli` was introduced in bitcoin@f6ade9c

ACKs for top commit:
  laanwj:
    Code review ACK 5ac804a

Tree-SHA512: 249c0b691a74cf201c729df86c3db2b3faefa53b94703941e566943d252c6d14924e935a8da4f592951574235923fbb7cd22612a5e7e02ff6c762c55a2320ca3
fanquake added a commit that referenced this pull request Dec 12, 2019
… 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
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Dec 12, 2019
…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
HashUnlimited pushed a commit to HashUnlimited/chaincoin that referenced this pull request Apr 17, 2020
jasonbcox pushed a commit to Bitcoin-ABC/bitcoin-abc that referenced this pull request Nov 10, 2020
… such

Summary:
The option `--usecli` was added to test_framework in [[bitcoin/bitcoin#11970 | PR11970]]. Running the test with this parameter will cause all RPCs to be sent through `bitcoin-cli` rather than directly over http. But the test were not all annotated accordingly. 74 tests were unnecessarily skipped.

Explicitely annotating the tests that do not support this option will allow to make this lack of support the exception, and identify these tests clearly as needing to be fixed in  the future.

This commit has currently no effect, as `self.supports_cli` is still considered `False` by default. This will be changed in the next commit.

In addition to the tests skipped in the Core PR, I also found the following tests to not support `--usecli`:
- `abc_wallet_standardness`: `TypeError: Object of type 'Decimal' is not JSON serializable` (will be fixed when backporting PR17705)
- `abc-magnetic-anomaly-mining`: idem
- `p2p_blocksonly`: idem
- `rpc_whitelist`: `AttributeError: 'NoneType' object has no attribute 'rfind'` (this test was added after PR17675 and should have been skipped in Core as well)
- `abc_p2p_fullblocktest`: `OSError: [Errno 7] Argument list too long: '.../bitcoin-abc/build/src/bitcoin-cli'`
- `wallet_groups` : idem

This is a backport of Core [[bitcoin/bitcoin#17675 | PR17675]] [1/2]
bitcoin/bitcoin@993e38a

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

Reviewers: #bitcoin_abc, deadalnix

Reviewed By: #bitcoin_abc, deadalnix

Subscribers: deadalnix

Differential Revision: https://reviews.bitcoinabc.org/D8316
jasonbcox pushed a commit to Bitcoin-ABC/bitcoin-abc that referenced this pull request Nov 10, 2020
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
…d when running test_runner.py --usecli

5ac804a tests: Use a default of supports_cli=True (instead of supports_cli=False) (practicalswift)
993e38a tests: Mark functional tests not supporting bitcoin-cli (--usecli) as such (practicalswift)

Pull request description:

  Annotate functional tests supporting `bitcoin-cli` (`--usecli`) as such.

  Prior to this commit 74 tests were unnecessarily skipped when running `test_runner.py --usecli`.

  Before:

  ```
  $ test/functional/test_runner.py --usecli > /dev/null 2>&1
  $ echo $?
  0
  $ test/functional/test_runner.py --usecli 2>&1 | cut -f2 -d'|' | \
      grep -E ' (Passed|Skipped) *$' | sort | uniq -c
        9  ✓ Passed
      126  ○ Skipped
  ```

  After:

  ```
  $ test/functional/test_runner.py --usecli > /dev/null 2>&1
  $ echo $?
  0
  $ test/functional/test_runner.py --usecli 2>&1 | cut -f2 -d'|' | \
      grep -E ' (Passed|Skipped) *$' | sort | uniq -c
       83  ✓ Passed
       52  ○ Skipped
  ```

  Context: `--usecli` was introduced in bitcoin@f6ade9c

ACKs for top commit:
  laanwj:
    Code review ACK 5ac804a

Tree-SHA512: 249c0b691a74cf201c729df86c3db2b3faefa53b94703941e566943d252c6d14924e935a8da4f592951574235923fbb7cd22612a5e7e02ff6c762c55a2320ca3
sidhujag pushed a commit to syscoin-core/syscoin that referenced this pull request Nov 10, 2020
…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
backpacker69 pushed a commit to peercoin/peercoin that referenced this pull request Mar 28, 2021
@practicalswift practicalswift deleted the dash-dash-usecli branch April 10, 2021 19:39
Stackout pushed a commit to VeriBlock/vbk-ri-btc that referenced this pull request Feb 17, 2022
knst pushed a commit to knst/dash that referenced this pull request Jun 7, 2022
…d when running test_runner.py --usecli

5ac804a tests: Use a default of supports_cli=True (instead of supports_cli=False) (practicalswift)
993e38a tests: Mark functional tests not supporting bitcoin-cli (--usecli) as such (practicalswift)

Pull request description:

  Annotate functional tests supporting `bitcoin-cli` (`--usecli`) as such.

  Prior to this commit 74 tests were unnecessarily skipped when running `test_runner.py --usecli`.

  Before [bitcoin original commit stats]:

  ```
  $ test/functional/test_runner.py --usecli > /dev/null 2>&1
  $ echo $?
  0
  $ test/functional/test_runner.py --usecli 2>&1 | cut -f2 -d'|' | \
      grep -E ' (Passed|Skipped) *$' | sort | uniq -c
        9  ✓ Passed
      126  ○ Skipped
  ```

  After [dash numbers]:

  ```
  $ test/functional/test_runner.py --usecli > /dev/null 2>&1
  $ echo $?
  0
  $ test/functional/test_runner.py --usecli 2>&1 | cut -f2 -d'|' | \
      grep -E ' (Passed|Skipped) *$' | sort | uniq -c
       110  ✓ Passed
       51  ○ Skipped
  ```

  Context: `--usecli` was introduced in bitcoin@f6ade9c

ACKs for top commit:
  laanwj:
    Code review ACK 5ac804a

Tree-SHA512: 249c0b691a74cf201c729df86c3db2b3faefa53b94703941e566943d252c6d14924e935a8da4f592951574235923fbb7cd22612a5e7e02ff6c762c55a2320ca3
knst pushed a commit to knst/dash that referenced this pull request Jun 7, 2022
…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
knst pushed a commit to knst/dash that referenced this pull request Jun 8, 2022
…d when running test_runner.py --usecli

5ac804a tests: Use a default of supports_cli=True (instead of supports_cli=False) (practicalswift)
993e38a tests: Mark functional tests not supporting bitcoin-cli (--usecli) as such (practicalswift)

Pull request description:

  Annotate functional tests supporting `bitcoin-cli` (`--usecli`) as such.

  Prior to this commit 74 tests were unnecessarily skipped when running `test_runner.py --usecli`.

  Before [bitcoin original commit stats]:

  ```
  $ test/functional/test_runner.py --usecli > /dev/null 2>&1
  $ echo $?
  0
  $ test/functional/test_runner.py --usecli 2>&1 | cut -f2 -d'|' | \
      grep -E ' (Passed|Skipped) *$' | sort | uniq -c
        9  ✓ Passed
      126  ○ Skipped
  ```

  After [dash numbers]:

  ```
  $ test/functional/test_runner.py --usecli > /dev/null 2>&1
  $ echo $?
  0
  $ test/functional/test_runner.py --usecli 2>&1 | cut -f2 -d'|' | \
      grep -E ' (Passed|Skipped) *$' | sort | uniq -c
       110  ✓ Passed
       51  ○ Skipped
  ```

  Context: `--usecli` was introduced in bitcoin@f6ade9c

ACKs for top commit:
  laanwj:
    Code review ACK 5ac804a

Tree-SHA512: 249c0b691a74cf201c729df86c3db2b3faefa53b94703941e566943d252c6d14924e935a8da4f592951574235923fbb7cd22612a5e7e02ff6c762c55a2320ca3
knst pushed a commit to knst/dash that referenced this pull request Jun 8, 2022
…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 Aug 16, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants