Page MenuHomePhabricator

[qa] fix p2p-fullblocktest.py to support magnetic anomaly
AbandonedPublic

Authored by jasonbcox on Nov 18 2018, 08:33.

Details

Reviewers
Fabien
schancel
deadalnix
Group Reviewers
Restricted Project
Summary

Per title, failures associated with the activation of CTOR, CLEANSTACK,
and a change in error message introduced in D2076

Test Plan
make check && ./test/functional/test_runner.py p2p-fullblocktest

Diff Detail

Repository
rABC Bitcoin ABC
Branch
p2p-fullblocktest
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 3950
Build 5972: Bitcoin ABC Buildbot (legacy)
Build 5971: arc lint + arc unit

Event Timeline

schancel retitled this revision from [qa] WIP fix p2p-fullblocktest.py to support magnetic anomaly to [qa] fix p2p-fullblocktest.py to support magnetic anomaly.Nov 18 2018, 08:51
Fabien requested changes to this revision.Nov 18 2018, 10:11
Fabien added a subscriber: Fabien.
Fabien added inline comments.
src/validation.cpp
3355 ↗(On Diff #5880)

bad-txns-duplicate is already raised on validation.cpp:3122, better change the error name to avoid confusion. From my understanding, the 2 errors are slightly different.

test/functional/test_framework/blocktools.py
31 ↗(On Diff #5880)

Good idea !

This revision now requires changes to proceed.Nov 18 2018, 10:11
jasonbcox requested changes to this revision.Nov 18 2018, 15:15
jasonbcox added a subscriber: jasonbcox.
jasonbcox added inline comments.
test/functional/p2p-fullblocktest.py
561 ↗(On Diff #5880)

numTxes -> numTxs

565 ↗(On Diff #5880)

can be simplified to for i in range(numTxs):

613 ↗(On Diff #5880)

Why the comment if it's accepted?

src/validation.cpp
3355 ↗(On Diff #5880)

This doesn't look related to making the test green. Please focus on that first.

test/functional/p2p-fullblocktest.py
565 ↗(On Diff #5880)

nvm

jasonbcox edited reviewers, added: schancel; removed: jasonbcox.

Fixed according to feedback

jasonbcox added inline comments.
test/functional/p2p-fullblocktest.py
1311 ↗(On Diff #5887)

This looks wrong, but passes. I don't know why yet.

deadalnix requested changes to this revision.Nov 18 2018, 18:28
deadalnix added a subscriber: deadalnix.
deadalnix added inline comments.
test/functional/p2p-fullblocktest.py
561 ↗(On Diff #5887)

Ok this is the worst name ever. This is not necessarily creating a long chain, nor it is the goal at all, because we are testing sigops.

1041 ↗(On Diff #5887)

When you see things like this, you should move the comment on the previous line so that formatting isn't screwed up.

This revision now requires changes to proceed.Nov 18 2018, 18:28
test/functional/p2p-fullblocktest.py
608 ↗(On Diff #5894)

This changes completely what this test is doing, why ?

793 ↗(On Diff #5887)

Revert

src/validation.cpp
3355 ↗(On Diff #5880)

This *is* a change in the error behavior. It changes the external API.

test/functional/p2p-fullblocktest.py
608 ↗(On Diff #5894)

No. It does the same thing AFAICS. The old version was invalid because of LTOR changing transaction indices in the block. So, I change it to generate the same thing twice.

test/functional/p2p-fullblocktest.py
1311 ↗(On Diff #5887)

I didn't add this. Where did it come from?

613 ↗(On Diff #5880)

It was failing with that. I forgot to remove the comment.