Page MenuHomePhabricator

[tests] Functional test naming convention
ClosedPublic

Authored by deadalnix on Apr 17 2020, 00:48.

Details

Summary
  • [tests] [docs] update README for new test naming scheme
  • [tests] README.md nit fixes
  • [tests] Check tests conform to naming convention

Extra-Author: John Newbery <john@johnnewbery.com>

This is a backport of Core PR11796

Test Plan
ninja check-functional

Diff Detail

Repository
rABC Bitcoin ABC
Branch
pr11796
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 10271
Build 18364: Default Diff Build & Tests
Build 18363: arc lint + arc unit

Event Timeline

[Bot Message]
One or more PR numbers were detected in the summary.
Links to those PRs have been inserted into the summary for reference.

Fabien requested changes to this revision.Apr 17 2020, 08:55
Fabien added a subscriber: Fabien.
Fabien added inline comments.
doc/functional-tests.md
239 ↗(On Diff #18878)

Please add a line for the abc-* tests.
Maybe it's time to rename them all to abc_* (but not in this diff).

288 ↗(On Diff #18878)

p2p-acceptblock.py doesn't exist (still p2p_unrequested_blocks.py, is there a missing backport ?), and the renaming is in contradiction with the naming convention you just added.

test/functional/test_runner.py
607 ↗(On Diff #18878)

I know this is from the PR, but does it makes sense for us ? IMO this is just making the whole check useless.

610 ↗(On Diff #18878)

Add abc

This revision now requires changes to proceed.Apr 17 2020, 08:55

I got a warning during the build:

[148/218] Building CXX object src/wallet/CMakeFiles/wallet-tool.dir/wallettool.cpp.o
../src/wallet/wallettool.cpp: In function ‘std::shared_ptr<CWallet> WalletTool::LoadWallet(const string&, const boost::filesystem::path&)’:
../src/wallet/wallettool.cpp:73:25: warning: catching polymorphic type ‘const class std::runtime_error’ by value [-Wcatch-value=]
     } catch (const std::runtime_error) {
                         ^~~~~~~~~~~~~
deadalnix added inline comments.
test/functional/test_runner.py
607 ↗(On Diff #18878)

It make the whole thing very useful. Like we don't have to address al the abc right away, and so this can go in without a bunch of extra changes.

test/functional/test_runner.py
607 ↗(On Diff #18878)

That's already the role of EXPECTED_VIOLATION_COUNT, where 29 is the number of abc-* tests.
My understanding is that the check will only fail if we add > 10 more tests that break the rule.

test/functional/test_runner.py
607 ↗(On Diff #18878)

Fee free to takeover https://github.com/bitcoin/bitcoin/pull/12252 if you want. We won't go faster through backport by going slower.

610 ↗(On Diff #18878)

No. It's a bad scheme and as the number of tests increase, we want to have them use proper names.

Fabien added inline comments.
test/functional/test_runner.py
607 ↗(On Diff #18878)

OK, I didn't notice it will be removed in a later PR

610 ↗(On Diff #18878)

OK

This revision is now accepted and ready to land.Apr 17 2020, 14:10