Details

Reviewers

Group Reviewers

Restricted Project

Commits

rSTAGING4adf7c7104e8: Added support for -gravitonactivationtime to unit tests
rABC4adf7c7104e8: Added support for -gravitonactivationtime to unit tests

Summary

This will help prevent situations like D4451 where unit tests fail after an upgrade.

The usage of the graviton upgrade for this diff is after-the-fact because the rest of the codebase has
not yet been migrated to the next upgrade name/time, and this allows for easy verification that this
patch works as expected (see test plan).

This patch has only been prepared as a new feature for the CMake build only as we're beginning to
put Autotools on maintenance-only.

Depends on D4462 and D4496

Test Plan

cmake -GNinja -DTEST_UPGRADE=On ..
ninja check  # passes

arc patch --skip-dependencies D4462
arc patch --skip-dependencies D4474  # this patch
git checkout 98d6c53  # the commit before D4451
git cherry-pick arcpatch-D4462
git cherry-pick arcpatch-D4474
ninja check  # fails
<build>/src/test/test_bitcoin -gravitonactivationtime=1585000000  # a time in the future so graviton is disabled; passes

Diff Detail

Repository

rABC Bitcoin ABC

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

jasonbcox created this revision.Nov 19 2019, 18:07

Herald added a reviewer: deadalnix. · View Herald TranscriptNov 19 2019, 18:07

Build Bitcoin-ABC / Diffs / Diff Testing started.

jasonbcox edited the test plan for this revision. (Show Details)Nov 19 2019, 18:07

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

Build Bitcoin-ABC / Diffs / Diff Testing passed.

Harbormaster completed remote builds in B8162: Diff 14208.Nov 19 2019, 18:17

You just removed the ability to use just about 90% of boost's unit tests abilities.

This revision now requires changes to proceed.Nov 20 2019, 13:30

Added all boost runtime params to the whitelist

What about using this: https://www.boost.org/doc/libs/1_71_0/libs/test/doc/html/boost_test/runtime_config/custom_command_line_arguments.html ?

You can then setup gArgs as you want it to in BasicTestingSetup.

src/test/test_bitcoin_main.cpp
69 ↗	(On Diff #14259)	This isn't going to be very sustainable, is it?

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

Build Bitcoin-ABC / Diffs / Diff Testing started.

Build Bitcoin-ABC / Diffs / Diff Testing passed.

Harbormaster completed remote builds in B8191: Diff 14259.Nov 20 2019, 18:55

In D4474#108521, @deadalnix wrote:

What about using this: https://www.boost.org/doc/libs/1_71_0/libs/test/doc/html/boost_test/runtime_config/custom_command_line_arguments.html ?

You can then setup gArgs as you want it to in BasicTestingSetup.

Unfortunately this doesn't work because not all tests use BasicTestingSetup (nor do they need to). It looks like there's a better way to do this using gArgs.ForceSetArg() rather than gArgs.ParseParameters(), but it will require a little refactoring.

Use ForceSetArg() instead of ParseParameters()

Build Bitcoin-ABC / Diffs / Diff Testing started.

jasonbcox edited the summary of this revision. (Show Details)Nov 20 2019, 20:29

jasonbcox added a parent revision: D4496: Split out key-value parsing in ArgsManager into its own function.

Build Bitcoin-ABC / Diffs / Diff Testing failed.

Harbormaster failed remote builds in B8197: Diff 14267!Nov 20 2019, 20:40

deadalnix accepted this revision.Nov 20 2019, 22:13

deadalnix added inline comments.

src/test/CMakeLists.txt
5 ↗	(On Diff #14267)	It's kind interesting that the comment doesn't say "Test the next upgrade" or something similar but instead "Test with the next upgrade activated".
src/test/test_bitcoin_main.cpp
31 ↗	(On Diff #14267)	remove envp, it's not portable

This revision is now accepted and ready to land.Nov 20 2019, 22:13

jasonbcox added inline comments.Nov 20 2019, 22:33

src/test/CMakeLists.txt
5 ↗	(On Diff #14267)	There's a subtle difference. "Test the next upgrade" could imply that it's testing activation. While we happen to have a test that does this, this isn't necessarily the case. We're running the unit tests such that the upgrade is already activated.