Page MenuHomePhabricator

Add a separate executable for seeder tests
Needs ReviewPublic

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
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 8043
Build 14081: Bitcoin ABC Buildbot (legacy)
Build 14080: arc lint + arc unit

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
jasonbcox added inline comments.Nov 9 2019, 01:18
src/Makefile.seedertest.include
16 ↗(On Diff #14019)

This needs cleaning up. Most of those are not and will not be used in the seeder.

src/seeder/test/CMakeLists.txt
1 ↗(On Diff #14019)

needs all three lines of the copyright

src/seeder/test/seeder_tests.cpp
12 ↗(On Diff #14019)

nit: you can leave out the dummy line. doesn't really matter as long as you remove it later though.

This revision now requires changes to proceed.Nov 9 2019, 01:18
nakihito updated this revision to Diff 14023.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

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

13

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

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.Wed, Nov 13, 18:05
nakihito edited the test plan for this revision. (Show Details)
Fabien requested changes to this revision.Wed, Nov 20, 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.Wed, Nov 20, 07:49
nakihito updated this revision to Diff 14268.Wed, Nov 20, 20:37

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

Fabien requested changes to this revision.Fri, Nov 22, 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.Fri, Nov 22, 07:29
nakihito updated this revision to Diff 14332.Fri, Nov 22, 19:06

Removed unnecessary line.

Fabien requested changes to this revision.Sun, Nov 24, 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.Sun, Nov 24, 10:29
nakihito updated this revision to Diff 14400.Mon, Nov 25, 19:25

Renamed test stub.

nakihito planned changes to this revision.Mon, Nov 25, 19:25
nakihito requested review of this revision.Mon, Nov 25, 20:07
Fabien accepted this revision.Tue, Nov 26, 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.Wed, Dec 4, 23:41

Rebase.

Fabien requested changes to this revision.Wed, Dec 11, 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.Wed, Dec 11, 15:48
nakihito updated this revision to Diff 14821.Fri, Dec 13, 01:08

Changed test name seeder -> seeder_tests.

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

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