Page MenuHomePhabricator
Differential D2085

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

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
arcpatch-D2085
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 3962
Build 5996: Bitcoin ABC Buildbot (legacy)
Build 5995: arc lint + arc unit

Event Timeline

schancel created this revision.Nov 18 2018, 08:33
Herald added a reviewer: Restricted Project. · View Herald TranscriptNov 18 2018, 08:33
teamcity added a subscriber: teamcity.Nov 18 2018, 08:35

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing started.

teamcity added a comment.Nov 18 2018, 08:41

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing failed.

Harbormaster failed remote builds in B3950: Diff 5879!Nov 18 2018, 08:41
schancel updated this revision to Diff 5880.Nov 18 2018, 08:50

Fix last test

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
teamcity added a comment.Nov 18 2018, 08:52

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing started.

teamcity added a comment.Nov 18 2018, 08:58

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing failed.

Harbormaster failed remote builds in B3951: Diff 5880!Nov 18 2018, 08:58
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
Fabien added a child revision: D2086: [qa] fix abc-p2p-fullblock test to support magnetic anomaly.Nov 18 2018, 10:16
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?

jasonbcox added inline comments.Nov 18 2018, 16:25
src/validation.cpp
3355 ↗(On Diff #5880)

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

jasonbcox added inline comments.Nov 18 2018, 16:32
test/functional/p2p-fullblocktest.py
565 ↗(On Diff #5880)

nvm

jasonbcox commandeered this revision.Nov 18 2018, 16:33
jasonbcox edited reviewers, added: schancel; removed: jasonbcox.
jasonbcox updated this revision to Diff 5887.Nov 18 2018, 17:05

Fixed according to feedback

jasonbcox marked an inline comment as done.Nov 18 2018, 17:06
jasonbcox added inline comments.
test/functional/p2p-fullblocktest.py
1311 ↗(On Diff #5887)

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

teamcity added a comment.Nov 18 2018, 17:07

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing started.

teamcity added a comment.Nov 18 2018, 17:13

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing failed.

Harbormaster failed remote builds in B3957: Diff 5887!Nov 18 2018, 17:13
jasonbcox removed a child revision: D2086: [qa] fix abc-p2p-fullblock test to support magnetic anomaly.Nov 18 2018, 17:39
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
jasonbcox updated this revision to Diff 5894.Nov 18 2018, 18:49

Feedback

teamcity added a comment.Nov 18 2018, 18:50

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing started.

teamcity added a comment.Nov 18 2018, 18:57

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing failed.

Harbormaster failed remote builds in B3962: Diff 5894!Nov 18 2018, 18:57
deadalnix added inline comments.Nov 18 2018, 19:02
test/functional/p2p-fullblocktest.py
608

This changes completely what this test is doing, why ?

793 ↗(On Diff #5887)

Revert

schancel added inline comments.Nov 18 2018, 19:11
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

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.

schancel added inline comments.Nov 18 2018, 19:16
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.

jasonbcox abandoned this revision.Nov 18 2018, 19:37

Superseded by D2088

Revision Contents
Changeset List

PathSize
test/
functional/
99 lines

Diff 5894

View Options

test/functional/p2p-fullblocktest.py

Loading...