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] remove redundant univalue_tests.cpp #11879

Merged
merged 1 commit into from Dec 20, 2017

Conversation

jnewbery
Copy link
Contributor

univalue unit tests were added in #4730 , and exist at /src/test/univalue_tests.cpp (outside the univalue tree). That test was brought into the univalue repository in bitcoin-core/univalue-subtree#4 , which was pulled into the github repository in #11420.

That means that the univalue test exists in two places:

  1. /src/test/univalue_tests.cpp
  2. /src/univalue/test/object.cpp

(2) is a strict superset of (1). It adds some macros to work around boost not being a univalue dependency, and adds a few extra lines of test.

Therefore remove /src/test/univalue_tests.cpp

@practicalswift
Copy link
Contributor

Oh, nice catch! -334 lines!

utACK 2862b56

@maflcko
Copy link
Member

maflcko commented Dec 12, 2017

Concept ACK. Going to review after a subtree pull

@laanwj
Copy link
Member

laanwj commented Dec 13, 2017

Going to review after a subtree pull

What needs to be pulled in first?

@maflcko
Copy link
Member

maflcko commented Dec 13, 2017

The tests would break by merging it right now. See bitcoin-core/univalue-subtree#9

@jnewbery
Copy link
Contributor Author

The tests would break...

More accurately, the tests wouldn't be fully testing what we expect them to. They'd continue to pass.

univalue doesn't change frequently, and as long as we don't pull in unrelated changes from univalue without pulling in bitcoin-core/univalue-subtree#9, there's no risk or downside to merging this PR.

@maflcko
Copy link
Member

maflcko commented Dec 13, 2017

no risk or downside to merging this PR

I disagree. There might be no risk, but there is a downside. One tool for testing changes is to break the software (or test) on purpose and see if the test (or software), i.e. the "counterpart", fails. Merging the pull request now would make it infeasible to use this approach for testing on univalue objects.

Copy link
Contributor

@ryanofsky ryanofsky left a comment

Choose a reason for hiding this comment

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

utACK 2862b56. Confirmed test/univalue_tests.cpp is a subset of univalue/test/object.cpp, though it seems #11952 should be merged before this PR.

@laanwj laanwj merged commit 2862b56 into bitcoin:master Dec 20, 2017
laanwj added a commit that referenced this pull request Dec 20, 2017
2862b56 [tests] remove redundant univalue_tests.cpp (John Newbery)

Pull request description:

  univalue unit tests were added in #4730 , and exist at `/src/test/univalue_tests.cpp` (outside the univalue tree). That test was brought into the univalue repository in bitcoin-core/univalue-subtree#4 , which was pulled into the github repository in #11420.

  That means that the univalue test exists in two places:
  1. `/src/test/univalue_tests.cpp`
  2. `/src/univalue/test/object.cpp`

  (2) is a strict superset of (1). It adds some macros to work around boost not being a univalue dependency, and adds a few extra lines of test.

  Therefore remove `/src/test/univalue_tests.cpp`

Tree-SHA512: 3747b10bbf62e9f12363905488b29945ad559ddca68c5c03d8a362de612a51f408f41a04d3712c6889bfc1632fb1a5fa0d7df0fbf02c322b3981a6d698f501b0
@jnewbery jnewbery deleted the remove_univalue_test branch December 20, 2017 15:54
@maflcko
Copy link
Member

maflcko commented Dec 20, 2017

Tested that the tests work on current master, now.

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512

Tested ACK 2862b562cc17f9d4507dab3b9281bf066b093e16
-----BEGIN PGP SIGNATURE-----

iQIcBAEBCgAGBQJaOthVAAoJENLqSFDnUoslQRwP/0Vx+fszR58LBm5xAeQwITHI
ZSxnSAuO3n069DLSeBKKHxDLwkBLyAtlgbJew5L3pG/XD7bCOWBz46mJmZFIC+yf
g/UswPICh5tU/hl+UBcAQWMtxHWAWOpyl+MRJuNAfi3bWLzzJukT4zgSq39WeGEi
rV23M+zNhTvLAMlkvstQeL18zVetxNgQl+q1WnV5odykIZuXaoeJOppWTu4ev8Os
C+T2xpNFtRazi1Zao5ouR2siqFydCoMUhFJT6A6kM4tPWZg9QdQKzMtC4LDJkVBC
VUZKlSAmaZUGoucHJbFoe+dqRu1Jctc2uBlKwC9VmDWj71JqSu8PoQ0U70Kk81iV
Vu7CgxVBLdErAzSeG+VGBHHF7jiM/yVBIQrkmU5pu0KDOxUi1XQaAu9Qv2852Q81
1Zcyw36RCzfKe6gSvCO3NU2o26PdZCLt/GYDWtx4CO0CoDP4rTEde2PACj9j2LCC
T1OT6jIN72Is+krlY7Y/gPb9neKeLCSnp8NDzfotJGTeC8BRmIejoWXYcxG1OJvR
sAi5JhVOvD2GClVZuAuYiTfY427w5zNnSf6UIVEaJ8saojOslR0uA36NBbiMr4Qk
dFN9e+z3RgrGdIfg4ykPX4qO2/M+xecq5RPcL1tbwqcXiNIWV4I8ydC65PIMlU9u
uBuruRLsWGGpyNZqPKpM
=3ItC
-----END PGP SIGNATURE-----

jasonbcox pushed a commit to Bitcoin-ABC/bitcoin-abc that referenced this pull request Jul 25, 2019
Summary:
2862b56 [tests] remove redundant univalue_tests.cpp (John Newbery)

Pull request description:

  univalue unit tests were added in #4730 , and exist at `/src/test/univalue_tests.cpp` (outside the univalue tree). That test was brought into the univalue repository in bitcoin-core/univalue-subtree#4 , which was pulled into the github repository in bitcoin/bitcoin#11420.

  That means that the univalue test exists in two places:
  1. `/src/test/univalue_tests.cpp`
  2. `/src/univalue/test/object.cpp`

  (2) is a strict superset of (1). It adds some macros to work around boost not being a univalue dependency, and adds a few extra lines of test.

  Therefore remove `/src/test/univalue_tests.cpp`

Tree-SHA512: 3747b10bbf62e9f12363905488b29945ad559ddca68c5c03d8a362de612a51f408f41a04d3712c6889bfc1632fb1a5fa0d7df0fbf02c322b3981a6d698f501b0

Backport of Core PR11879
bitcoin/bitcoin#11879

Test Plan:
  make check
  ninja check

Reviewers: deadalnix, Fabien, jasonbcox, O1 Bitcoin ABC, #bitcoin_abc

Reviewed By: jasonbcox, O1 Bitcoin ABC, #bitcoin_abc

Differential Revision: https://reviews.bitcoinabc.org/D3701
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Jan 17, 2020
2862b56 [tests] remove redundant univalue_tests.cpp (John Newbery)

Pull request description:

  univalue unit tests were added in dashpay#4730 , and exist at `/src/test/univalue_tests.cpp` (outside the univalue tree). That test was brought into the univalue repository in bitcoin-core/univalue-subtree#4 , which was pulled into the github repository in bitcoin#11420.

  That means that the univalue test exists in two places:
  1. `/src/test/univalue_tests.cpp`
  2. `/src/univalue/test/object.cpp`

  (2) is a strict superset of (1). It adds some macros to work around boost not being a univalue dependency, and adds a few extra lines of test.

  Therefore remove `/src/test/univalue_tests.cpp`

Tree-SHA512: 3747b10bbf62e9f12363905488b29945ad559ddca68c5c03d8a362de612a51f408f41a04d3712c6889bfc1632fb1a5fa0d7df0fbf02c322b3981a6d698f501b0
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Jan 22, 2020
2862b56 [tests] remove redundant univalue_tests.cpp (John Newbery)

Pull request description:

  univalue unit tests were added in dashpay#4730 , and exist at `/src/test/univalue_tests.cpp` (outside the univalue tree). That test was brought into the univalue repository in bitcoin-core/univalue-subtree#4 , which was pulled into the github repository in bitcoin#11420.

  That means that the univalue test exists in two places:
  1. `/src/test/univalue_tests.cpp`
  2. `/src/univalue/test/object.cpp`

  (2) is a strict superset of (1). It adds some macros to work around boost not being a univalue dependency, and adds a few extra lines of test.

  Therefore remove `/src/test/univalue_tests.cpp`

Tree-SHA512: 3747b10bbf62e9f12363905488b29945ad559ddca68c5c03d8a362de612a51f408f41a04d3712c6889bfc1632fb1a5fa0d7df0fbf02c322b3981a6d698f501b0
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Jan 22, 2020
2862b56 [tests] remove redundant univalue_tests.cpp (John Newbery)

Pull request description:

  univalue unit tests were added in dashpay#4730 , and exist at `/src/test/univalue_tests.cpp` (outside the univalue tree). That test was brought into the univalue repository in bitcoin-core/univalue-subtree#4 , which was pulled into the github repository in bitcoin#11420.

  That means that the univalue test exists in two places:
  1. `/src/test/univalue_tests.cpp`
  2. `/src/univalue/test/object.cpp`

  (2) is a strict superset of (1). It adds some macros to work around boost not being a univalue dependency, and adds a few extra lines of test.

  Therefore remove `/src/test/univalue_tests.cpp`

Tree-SHA512: 3747b10bbf62e9f12363905488b29945ad559ddca68c5c03d8a362de612a51f408f41a04d3712c6889bfc1632fb1a5fa0d7df0fbf02c322b3981a6d698f501b0
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Jan 29, 2020
2862b56 [tests] remove redundant univalue_tests.cpp (John Newbery)

Pull request description:

  univalue unit tests were added in dashpay#4730 , and exist at `/src/test/univalue_tests.cpp` (outside the univalue tree). That test was brought into the univalue repository in bitcoin-core/univalue-subtree#4 , which was pulled into the github repository in bitcoin#11420.

  That means that the univalue test exists in two places:
  1. `/src/test/univalue_tests.cpp`
  2. `/src/univalue/test/object.cpp`

  (2) is a strict superset of (1). It adds some macros to work around boost not being a univalue dependency, and adds a few extra lines of test.

  Therefore remove `/src/test/univalue_tests.cpp`

Tree-SHA512: 3747b10bbf62e9f12363905488b29945ad559ddca68c5c03d8a362de612a51f408f41a04d3712c6889bfc1632fb1a5fa0d7df0fbf02c322b3981a6d698f501b0
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Jan 29, 2020
2862b56 [tests] remove redundant univalue_tests.cpp (John Newbery)

Pull request description:

  univalue unit tests were added in dashpay#4730 , and exist at `/src/test/univalue_tests.cpp` (outside the univalue tree). That test was brought into the univalue repository in bitcoin-core/univalue-subtree#4 , which was pulled into the github repository in bitcoin#11420.

  That means that the univalue test exists in two places:
  1. `/src/test/univalue_tests.cpp`
  2. `/src/univalue/test/object.cpp`

  (2) is a strict superset of (1). It adds some macros to work around boost not being a univalue dependency, and adds a few extra lines of test.

  Therefore remove `/src/test/univalue_tests.cpp`

Tree-SHA512: 3747b10bbf62e9f12363905488b29945ad559ddca68c5c03d8a362de612a51f408f41a04d3712c6889bfc1632fb1a5fa0d7df0fbf02c322b3981a6d698f501b0
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Jan 29, 2020
2862b56 [tests] remove redundant univalue_tests.cpp (John Newbery)

Pull request description:

  univalue unit tests were added in dashpay#4730 , and exist at `/src/test/univalue_tests.cpp` (outside the univalue tree). That test was brought into the univalue repository in bitcoin-core/univalue-subtree#4 , which was pulled into the github repository in bitcoin#11420.

  That means that the univalue test exists in two places:
  1. `/src/test/univalue_tests.cpp`
  2. `/src/univalue/test/object.cpp`

  (2) is a strict superset of (1). It adds some macros to work around boost not being a univalue dependency, and adds a few extra lines of test.

  Therefore remove `/src/test/univalue_tests.cpp`

Tree-SHA512: 3747b10bbf62e9f12363905488b29945ad559ddca68c5c03d8a362de612a51f408f41a04d3712c6889bfc1632fb1a5fa0d7df0fbf02c322b3981a6d698f501b0
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Jan 31, 2020
2862b56 [tests] remove redundant univalue_tests.cpp (John Newbery)

Pull request description:

  univalue unit tests were added in dashpay#4730 , and exist at `/src/test/univalue_tests.cpp` (outside the univalue tree). That test was brought into the univalue repository in bitcoin-core/univalue-subtree#4 , which was pulled into the github repository in bitcoin#11420.

  That means that the univalue test exists in two places:
  1. `/src/test/univalue_tests.cpp`
  2. `/src/univalue/test/object.cpp`

  (2) is a strict superset of (1). It adds some macros to work around boost not being a univalue dependency, and adds a few extra lines of test.

  Therefore remove `/src/test/univalue_tests.cpp`

Tree-SHA512: 3747b10bbf62e9f12363905488b29945ad559ddca68c5c03d8a362de612a51f408f41a04d3712c6889bfc1632fb1a5fa0d7df0fbf02c322b3981a6d698f501b0
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants