Skip to content

Conversation

domob1812
Copy link
Contributor

This adds explicit tests for the returned HTTP status codes to interface_rpc.py (for error cases) and the HTTP JSON-RPC client in general for success.

#15381 brought up discussion about the HTTP status codes in general, and the general opinion was that the current choice may not be ideal but should not be changed to preserve compatibility with existing JSON-RPC clients. Thus it makes sense to actually test the current status to ensure this desired compatibility is not broken accidentally.

@fanquake fanquake added the Tests label Feb 27, 2019
@domob1812 domob1812 changed the title Add regtests for HTTP status codes. Add regtests for HTTP status codes Feb 27, 2019
@DrahtBot
Copy link
Contributor

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #15483 (rpc: Adding a 'logpath' entry to getrpcinfo by darosior)
  • #15381 (Optionally return HTTP_OK for RPC errors by domob1812)

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.

This adds explicit tests for the returned HTTP status codes to
interface_rpc.py (for error cases) and the HTTP JSON-RPC client in
general for success.

PR 15381 brought up discussion about the HTTP status codes in general,
and the general opinion was that the current choice may not be ideal
but should not be changed to preserve compatibility with existing
JSON-RPC clients.  Thus it makes sense to actually test the current
status to ensure this desired compatibility is not broken accidentally.
@practicalswift
Copy link
Contributor

Concept ACK

Copy link
Contributor

@promag promag left a comment

Choose a reason for hiding this comment

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

Concept ACK.

@laanwj
Copy link
Member

laanwj commented Mar 7, 2019

utACK 8f5d943

@promag
Copy link
Contributor

promag commented Mar 17, 2019

utACK 8f5d943.

@domob1812
Copy link
Contributor Author

Is this good to merge, or is anything left for me to do?

@jonasschnelli
Copy link
Contributor

utACK 8f5d943

@jonasschnelli jonasschnelli merged commit 8f5d943 into bitcoin:master Apr 8, 2019
jonasschnelli added a commit that referenced this pull request Apr 8, 2019
8f5d943 Add regtests for HTTP status codes. (Daniel Kraft)

Pull request description:

  This adds explicit tests for the returned HTTP status codes to `interface_rpc.py` (for error cases) and the HTTP JSON-RPC client in general for success.

  #15381 brought up discussion about the HTTP status codes in general, and the general opinion was that the current choice may not be ideal but should not be changed to preserve compatibility with existing JSON-RPC clients.  Thus it makes sense to actually test the current status to ensure this desired compatibility is not broken accidentally.

ACKs for commit 8f5d94:
  laanwj:
    utACK 8f5d943
  promag:
    utACK 8f5d943.
  jonasschnelli:
    utACK 8f5d943

Tree-SHA512: 82503ccd134dd9145304e95cb6c61755f100bee27593d567cdd5c0c554d47e7b06d937456cab04107f46f4984930355db65d5e711008a0b05f2b8feec9f2950e
@domob1812 domob1812 deleted the test-http-code branch April 8, 2019 07:56
jasonbcox pushed a commit to Bitcoin-ABC/bitcoin-abc that referenced this pull request Oct 5, 2020
Summary:
PR description:

> This adds explicit tests for the returned HTTP status codes to
> interface_rpc.py (for error cases) and the HTTP JSON-RPC client in
> general for success.
>
> [[bitcoin/bitcoin#15381 | PR15381]] brought up discussion about the HTTP status codes in general,
> and the general opinion was that the current choice may not be ideal
> but should not be changed to preserve compatibility with existing
> JSON-RPC clients.  Thus it makes sense to actually test the current
> status to ensure this desired compatibility is not broken accidentally.

This is a backport of Core [[bitcoin/bitcoin#15495 | PR15495]]

Test Plan: `ninja && test/functional/test_runner.py interface_rpc`

Reviewers: O1 Bitcoin ABC, #bitcoin_abc, Fabien

Reviewed By: O1 Bitcoin ABC, #bitcoin_abc, Fabien

Differential Revision: https://reviews.bitcoinabc.org/D7752
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Sep 11, 2021
8f5d943 Add regtests for HTTP status codes. (Daniel Kraft)

Pull request description:

  This adds explicit tests for the returned HTTP status codes to `interface_rpc.py` (for error cases) and the HTTP JSON-RPC client in general for success.

  bitcoin#15381 brought up discussion about the HTTP status codes in general, and the general opinion was that the current choice may not be ideal but should not be changed to preserve compatibility with existing JSON-RPC clients.  Thus it makes sense to actually test the current status to ensure this desired compatibility is not broken accidentally.

ACKs for commit 8f5d94:
  laanwj:
    utACK 8f5d943
  promag:
    utACK 8f5d943.
  jonasschnelli:
    utACK 8f5d943

Tree-SHA512: 82503ccd134dd9145304e95cb6c61755f100bee27593d567cdd5c0c554d47e7b06d937456cab04107f46f4984930355db65d5e711008a0b05f2b8feec9f2950e
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Sep 11, 2021
8f5d943 Add regtests for HTTP status codes. (Daniel Kraft)

Pull request description:

  This adds explicit tests for the returned HTTP status codes to `interface_rpc.py` (for error cases) and the HTTP JSON-RPC client in general for success.

  bitcoin#15381 brought up discussion about the HTTP status codes in general, and the general opinion was that the current choice may not be ideal but should not be changed to preserve compatibility with existing JSON-RPC clients.  Thus it makes sense to actually test the current status to ensure this desired compatibility is not broken accidentally.

ACKs for commit 8f5d94:
  laanwj:
    utACK 8f5d943
  promag:
    utACK 8f5d943.
  jonasschnelli:
    utACK 8f5d943

Tree-SHA512: 82503ccd134dd9145304e95cb6c61755f100bee27593d567cdd5c0c554d47e7b06d937456cab04107f46f4984930355db65d5e711008a0b05f2b8feec9f2950e
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Sep 12, 2021
8f5d943 Add regtests for HTTP status codes. (Daniel Kraft)

Pull request description:

  This adds explicit tests for the returned HTTP status codes to `interface_rpc.py` (for error cases) and the HTTP JSON-RPC client in general for success.

  bitcoin#15381 brought up discussion about the HTTP status codes in general, and the general opinion was that the current choice may not be ideal but should not be changed to preserve compatibility with existing JSON-RPC clients.  Thus it makes sense to actually test the current status to ensure this desired compatibility is not broken accidentally.

ACKs for commit 8f5d94:
  laanwj:
    utACK 8f5d943
  promag:
    utACK 8f5d943.
  jonasschnelli:
    utACK 8f5d943

Tree-SHA512: 82503ccd134dd9145304e95cb6c61755f100bee27593d567cdd5c0c554d47e7b06d937456cab04107f46f4984930355db65d5e711008a0b05f2b8feec9f2950e
@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.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants