Page MenuHomePhabricator

[tests] Fix checkinputs_test to work under magnetic anomaly
AbandonedPublic

Authored by jasonbcox on Nov 18 2018, 20:01.

Details

Reviewers
deadalnix
schancel
Group Reviewers
Restricted Project
Summary

As per title. Add CTOR ordering to test block construction. Use
NULLDUMMY instead of CLEANSTACK for checking consensus vs non-consensus
script cache behavior.

Test Plan
make -j10 VERBOSE=1 check

Diff Detail

Repository
rABC Bitcoin ABC
Branch
unit-test
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 3971
Build 6014: Bitcoin ABC Buildbot (legacy)
Build 6013: arc lint + arc unit

Event Timeline

jasonbcox added inline comments.
src/test/test_bitcoin.cpp
170 ↗(On Diff #5897)

Is this comment still relevant? Either I'm missing something or it's already done above.

This is a better approach toward fixing that test.

src/test/test_bitcoin.cpp
163 ↗(On Diff #5898)

I don't think this really warrant to modify the test framework.

src/test/txvalidationcache_tests.cpp
202 ↗(On Diff #5898)

It's the last item. You are pushing onto a stack so the item you push first end up being the last item on the stack.

Fix tests with hackiness. Should try to use P2SH

schancel retitled this revision from WIP fix unit tests to [tests] Fix checkinputs_test to work under magnetic anomaly.Nov 19 2018, 00:54

Fix whitespace and unneeded includes

jasonbcox requested changes to this revision.Nov 19 2018, 01:10
jasonbcox added inline comments.
src/test/txvalidationcache_tests.cpp
198 ↗(On Diff #5904)

Either comment is out of date, or this appears to be missing OP_1

This revision now requires changes to proceed.Nov 19 2018, 01:10
src/test/txvalidationcache_tests.cpp
198 ↗(On Diff #5904)

Oh I see, I think this comment needs to be moved above ^^

deadalnix requested changes to this revision.Nov 19 2018, 01:12
deadalnix added inline comments.
src/script/interpreter.cpp
1084 ↗(On Diff #5903)

Revert

1088 ↗(On Diff #5903)

revert

src/test/txvalidationcache_tests.cpp
185 ↗(On Diff #5903)

There is no point in bringing the key store in there. You are building the script to begin with so anything you'll pull of the store you already have.

188 ↗(On Diff #5903)

I'm not sure why you want to do P2SH here instead of bare multisig. If there is a reason, it should be mentioned. if there is no reason, then don't do it.

200 ↗(On Diff #5903)

This would benefit being scoped to destruct the temporaries.

202 ↗(On Diff #5903)

This is not spending, this is mining.

243 ↗(On Diff #5903)

You can sign the exact same ways as before. You just need to push the non nulldummy element.

schancel marked an inline comment as done.

Fix comments

src/test/txvalidationcache_tests.cpp
243 ↗(On Diff #5903)

+1. doing this a different way is confusing

schancel added inline comments.
src/test/txvalidationcache_tests.cpp
243 ↗(On Diff #5903)

I did that

jasonbcox abandoned this revision.
jasonbcox edited reviewers, added: schancel; removed: jasonbcox.