Page MenuHomePhabricator

Add a separate executible for seeder tests
Changes PlannedPublic

Authored by nakihito on Sat, Nov 9, 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
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 8069
Build 14128: Bitcoin ABC Buildbot
Build 14127: arc lint + arc unit

Event Timeline

nakihito created this revision.Sat, Nov 9, 00:59
jasonbcox requested changes to this revision.Sat, Nov 9, 01:18
jasonbcox added inline comments.
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.Sat, Nov 9, 01:18
nakihito updated this revision to Diff 14023.Sat, Nov 9, 02:07

Added copyright lines and cleaned up Makefile.

deadalnix requested changes to this revision.Sun, Nov 10, 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.Sun, Nov 10, 15:32
nakihito updated this revision to Diff 14064.Tue, Nov 12, 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.Tue, Nov 12, 02:10
nakihito edited the summary of this revision. (Show Details)Tue, Nov 12, 02:16
nakihito edited the test plan for this revision. (Show Details)