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 OK
Unit
No Unit Test Coverage
Build Status
Buildable 8089
Build 14167: Bitcoin ABC Buildbot (legacy)
Build 14166: arc lint + arc unit

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
nakihito added a comment.Nov 9 2019, 02:07

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
nakihito updated this revision to Diff 14064.Nov 12 2019, 02:10

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

nakihito planned changes to this revision.Nov 12 2019, 02:10
nakihito edited the summary of this revision. (Show Details)Nov 12 2019, 02:16
nakihito edited the test plan for this revision. (Show Details)
nakihito updated this revision to Diff 14090.Nov 12 2019, 23:17
nakihito edited the test plan for this revision. (Show Details)

Rebased and adjusted some cmake names.

nakihito planned changes to this revision.Nov 13 2019, 00:55
jasonbcox accepted this revision.Nov 13 2019, 00:56
nakihito updated this revision to Diff 14096.Nov 13 2019, 01:11

Fixed typo.

nakihito planned changes to this revision.Nov 13 2019, 02:17
nakihito updated this revision to Diff 14098.Nov 13 2019, 02:19

Added $(PIE_FLAGS) for clang.

nakihito planned changes to this revision.Nov 13 2019, 02:19
nakihito requested review of this revision.Nov 13 2019, 18:05
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

Why is univalue required here ?

src/seeder/CMakeLists.txt
17

This seems unnecessary

This revision now requires changes to proceed.Nov 20 2019, 07:49
nakihito updated this revision to Diff 14268.Nov 20 2019, 20:37

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
nakihito updated this revision to Diff 14332.Nov 22 2019, 19:06

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 updated this revision to Diff 14400.Nov 25 2019, 19:25

Renamed test stub.

nakihito planned changes to this revision.Nov 25 2019, 19:25
nakihito requested review of this revision.Nov 25 2019, 20:07
Fabien accepted this revision.Nov 26 2019, 10:15
nakihito retitled this revision from Add a separate executible for seeder tests to Add a separate executable for seeder tests.
nakihito updated this revision to Diff 14633.Dec 4 2019, 23:41

Rebase.

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
nakihito updated this revision to Diff 14821.Dec 13 2019, 01:08

Changed test name seeder -> seeder_tests.

Fabien accepted this revision.Dec 13 2019, 08:31

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

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