Page MenuHomePhabricator

Add a separate executable for seeder tests
ClosedPublic

Authored by nakihito on Nov 9 2019, 00:59.

Details

Reviewers
deadalnix
Fabien
jasonbcox
Group Reviewers
Restricted Owners Package(Owns No Changed Paths)
Restricted Project
Commits
rSTAGING8fc1ee3ece76: Add a separate executable for seeder tests
rABC8fc1ee3ece76: Add a separate executable for seeder tests
Summary

As requested in discussion in D4276. This allows us to run seeder unit tests
independently from the other unit tests run by test_bitcoin

Depends on D4432

Test Plan
../configure
make check
src/seeder/test_bitcoin-seeder
../configure CXX=clang++ CC=clang
make clean
make check
src/seeder/test_bitcoin-seeder

ninja
ninja check

These should build everything including the seeder and seeder tests.

ninja test_bitcoin-seeder

This should only build test_bitcoin-seeder

ninja check-bitcoin-seeder

This should only run the bitcoin-seeder tests.

Diff Detail

Repository
rABC Bitcoin ABC
Branch
SeederTestExec
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 8069
Build 14128: Bitcoin ABC Buildbot (legacy)
Build 14127: arc lint + arc unit

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

Added copyright lines and cleaned up Makefile.

deadalnix requested changes to this revision.Nov 10 2019, 15:32
deadalnix added inline comments.
src/seeder/CMakeLists.txt
8 ↗(On Diff #14023)

Clearly, you have a configuration problem with your text editor.

13 ↗(On Diff #14023)

This whole thing is very confusing. There is a seeder target and a bitcoin-seeder one. Does that mean the seeder target contain all the logic of the seeder, at the exclusion of bitcoin specific ones? If so why does it have a bitcoin.cpp file in it?

Why is that target defined in the middle of the seeder's definition? They do not seem to even link with each other in the current state. Does this even produce a seeder that works at all?

src/seeder/test/CMakeLists.txt
32 ↗(On Diff #14023)

This is duplicated code and calls for some refactoring.

This revision now requires changes to proceed.Nov 10 2019, 15:32

Changed CMake names to be more descriptive, Updated copyright missing copyright lines, fixed formatting, and rebased to remove duplicate code.

nakihito edited the test plan for this revision. (Show Details)
nakihito edited the test plan for this revision. (Show Details)

Rebased and adjusted some cmake names.

Added $(PIE_FLAGS) for clang.

nakihito edited the test plan for this revision. (Show Details)
Fabien requested changes to this revision.Nov 20 2019, 07:49
Fabien added inline comments.
src/Makefile.seedertest.include
15 ↗(On Diff #14098)

Why is univalue required here ?

src/seeder/CMakeLists.txt
17 ↗(On Diff #14098)

This seems unnecessary

This revision now requires changes to proceed.Nov 20 2019, 07:49

Removed LIBUNIVALUE from autotools and removed unnecessary change in seeder/CMake.txt.

Fabien requested changes to this revision.Nov 22 2019, 07:29
Fabien added inline comments.
src/seeder/CMakeLists.txt
7 ↗(On Diff #14268)

It's not necessary at all

This revision now requires changes to proceed.Nov 22 2019, 07:29

Removed unnecessary line.

Fabien requested changes to this revision.Nov 24 2019, 10:29
Fabien added inline comments.
src/seeder/test/seeder_tests.cpp
11 ↗(On Diff #14332)

The test name obviously does not match the content

This revision now requires changes to proceed.Nov 24 2019, 10:29
nakihito retitled this revision from Add a separate executible for seeder tests to Add a separate executable for seeder tests.
Fabien requested changes to this revision.Dec 11 2019, 15:48

Please add the new test path to the lint-tests linter in the .arclint file, and fix the boost test suite name accordingly.

This revision now requires changes to proceed.Dec 11 2019, 15:48

Changed test name seeder -> seeder_tests.

This revealed a bug in the test linter, see D4718.

This revision is now accepted and ready to land.Dec 18 2019, 00:41
This revision was automatically updated to reflect the committed changes.