test: Always define the raii_event_tests test suite by candrews · Pull Request #16564 · bitcoin/bitcoin

candrews · 2019-08-07T15:38:53Z

The test suite must always be defined (even when EVENT_SET_MEM_FUNCTIONS_IMPLEMENTED is not defined) so that the test harness doesn't fail due to not being able to find the raii_event_tests test.

This improves upon 95f97f4 actually fixing #9493

kallewoof · 2019-08-07T15:54:17Z

Don't make pull requests into existing releases, make them into master.

Edit: the maintainers pull things into the tagged release versions as appropriate. Pull requests basically always go into master, unless they are specific to some release. Yours seems like a generic fix, although I recognize the gentoo downstream issue.

candrews · 2019-08-07T16:32:18Z

Don't make pull requests into existing releases, make them into master.

I've rebased this on master, thank you very much in advance

maflcko · 2019-08-07T16:48:02Z

Are there steps to reproduce the bug and see that this commit fixes it?

candrews · 2019-08-07T16:52:16Z

Are there steps to reproduce the bug and see that this commit fixes it?

Have libevent installed so that EVENT_SET_MEM_FUNCTIONS_IMPLEMENTED is not defined (on Gentoo, that's done by installing dev-libs/libevent with the debug flag unset.).
Then, build bitcoin and run the tests. You'll get this error output:

/var/tmp/portage/net-p2p/bitcoind-0.18.0-r1/work/bitcoin-2472733a24a9364e4c6233ccd04166a26a68cc65/src # test/test_bitcoin -l test_suite -t raii_event_creation
Test setup error: no test cases matching filter or all test cases were disabled

With this change, you don't get such a failure.

luke-jr · 2019-08-08T01:48:01Z

Conflicts

No conflicts as of last run.

luke-jr

Concept ACK

luke-jr · 2019-09-20T19:08:49Z

src/test/raii_event_tests.cpp

Suggest replacing with:

// It would probably be ideal to report skipped, but boost::test doesn't seem to make that practical (at least not in versions available with common distros) BOOST_TEST_MESSAGE("Skipping raii_event_tess: libevent doesn't support event_set_mem_functions");

luke-jr · 2019-09-20T19:09:24Z

src/test/raii_event_tests.cpp

Maybe consider making this an #else later on (or at least turn the existing #ifdef into an #else if we prefer the skip-test on top)

luke-jr · 2019-09-20T19:12:40Z

src/test/raii_event_tests.cpp

Also suggest renaming the test to raii_event_tests_SKIPPED

The test suite must always be defined (even when EVENT_SET_MEM_FUNCTIONS_IMPLEMENTED is not defined) so that the test harness doesn't fail due to not being able to find the raii_event_tests test. This improves upon 95f97f4 actually fixing bitcoin#9493

luke-jr · 2019-09-20T19:17:31Z

@candrews Here's a branch that can be force-pushed over this one if you like it:

master...luke-jr:raii_event_test_fix-0.14

(Cleanly merges to 0.14.0 through master)

candrews · 2019-09-20T19:19:37Z

@candrews Here's a branch that can be force-pushed over this one if you like it:

master...luke-jr:raii_event_test_fix-0.14

(Cleanly merges to 0.14.0 through master)

Awesome, that looks great!

maflcko · 2019-10-11T21:06:19Z

re-run ci

laanwj · 2020-02-05T15:41:45Z

I'd strongly prefer a solution that doesn't run raii_event_tests (and potentially other test suites) when disabled based on build configuration, instead of working around it by conditionally adding empty tests.

adamjonas · 2020-05-18T19:57:40Z

HI @candrews - bumping this to see if you'd like to continue to move this forward towards a solution that doesn't run raii_event_tests as requested above?

candrews · 2020-05-18T20:09:23Z

bumping this to see if you'd like to continue to move this forward towards a solution that doesn't run raii_event_tests as requested above?

If someone has a suggestion as to how to do that, I'd be happy to update this PR to implement it.

luke-jr · 2020-05-18T20:39:39Z

IMO we should just merge this until a better solution is implemented

maflcko · 2020-05-18T23:00:43Z

An alternative would be to kill xenial and use something like this: https://www.boost.org/doc/libs/1_59_0/libs/test/doc/html/boost_test/tests_organization/enabling.html

maflcko · 2020-05-18T23:04:48Z

https://packages.debian.org/jessie/libboost-all-dev is going to die in about a month https://wiki.debian.org/DebianReleases#Production_Releases

https://packages.ubuntu.com/xenial/libboost-all-dev is going to die in less than a year https://ubuntu.com/about/release-cycle

maflcko · 2020-05-18T23:11:45Z

Or another alternative (if raising the minimum version of boost is too controversial), could be to make the test skipable when a recent boost version is available or otherwise just make it mandatory and require EVENT_SET_MEM_FUNCTIONS_IMPLEMENTED? (I am assuming that EVENT_SET_MEM_FUNCTIONS_IMPLEMENTED is defined with xenial libevent?)

maflcko · 2020-05-31T12:55:23Z

The alternative was closed #16228 (comment), so this seems the best bet right now.

It can be improved upon later by bumping boost #16564 (comment), but that might or might not be controversial.

ACK

maflcko · 2020-05-31T21:55:36Z

Tested ACK:

Current master make check fails as if

$ ./src/test/test_bitcoin -t raii_event_tests
Test setup error: no test cases matching filter or all test cases were disabled

This pull:

$ ./src/test/test_bitcoin -t raii_event_tests
Running 1 test case...

*** No errors detected

ACK 9a19c9a 🎹

Show signature and timestamp

Signature:

Timestamp of file with hash 10409394f1acc2ff628bcbe5435062c7affa09d444d74bc8b734e1c9f9ba9f87 -

9a19c9a Always define the raii_event_tests test suite (Craig Andrews) Pull request description: The test suite must always be defined (even when EVENT_SET_MEM_FUNCTIONS_IMPLEMENTED is not defined) so that the test harness doesn't fail due to not being able to find the raii_event_tests test. This improves upon 95f97f4 actually fixing bitcoin#9493 ACKs for top commit: MarcoFalke: ACK 9a19c9a 🎹 Tree-SHA512: 3c42f17a9b5d56c8841f3aa9ac19da91c10aff210026266f31f7eb98a62528740d7c518c121452b68e8f801d6c80ecfb627d137ec6ed533289fa3beb08b4f176

Summary: PR description: > The test suite must always be defined (even when EVENT_SET_MEM_FUNCTIONS_IMPLEMENTED is not defined) so that the test harness doesn't fail due to not being able to find the raii_event_tests test. > > This improves upon 95f97f4 actually fixing bitcoin/bitcoin#9493 Context from issue: > [[bitcoin/bitcoin#9387 | core#9387]] added a new test that uses libevent's event_set_mem_functions which is sometimes not included with libevent. In particular, Gentoo only enables this when libevent is installed with the "debug" option. > Currently, this causes the build to simply fail on tests This is a backport of [[bitcoin/bitcoin#16564 | core#16564]] Test Plan: `ninja check` Reviewers: #bitcoin_abc, majcosta Reviewed By: #bitcoin_abc, majcosta Differential Revision: https://reviews.bitcoinabc.org/D9930

candrews mentioned this pull request Aug 7, 2019

Bitcoin 0.18.0 gentoo/gentoo#12059

Closed

candrews force-pushed the patch-1 branch 2 times, most recently from 2728557 to b2c8450 Compare August 7, 2019 16:31

candrews changed the base branch from 0.18 to master August 7, 2019 16:31

maflcko changed the title ~~Always define the raii_event_tests test suite~~ test: Always define the raii_event_tests test suite Aug 7, 2019

DrahtBot added the Tests label Aug 7, 2019

luke-jr suggested changes Sep 20, 2019

View reviewed changes

luke-jr mentioned this pull request Sep 20, 2019

Bugfix: make check: Only run tests that were compiled (& print correct log on failure) #16228

Closed

luke-jr reviewed Sep 20, 2019

View reviewed changes

src/test/raii_event_tests.cpp Outdated

Copy link

Member

luke-jr Sep 20, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also suggest renaming the test to raii_event_tests_SKIPPED

candrews force-pushed the patch-1 branch from b2c8450 to 9a19c9a Compare September 20, 2019 20:07

maflcko closed this Oct 11, 2019

maflcko reopened this Oct 11, 2019

maflcko closed this May 31, 2020

maflcko reopened this May 31, 2020

maflcko merged commit 091cc4b into bitcoin:master May 31, 2020

maflcko mentioned this pull request May 31, 2020

bump boost test to 1.59? #19128

Closed

bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022

Conversation

candrews commented Aug 7, 2019 • edited by maflcko Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kallewoof commented Aug 7, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

candrews commented Aug 7, 2019

Uh oh!

maflcko commented Aug 7, 2019

Uh oh!

candrews commented Aug 7, 2019

Uh oh!

luke-jr commented Aug 8, 2019

Uh oh!

maflcko commented Aug 8, 2019

Uh oh!

laanwj commented Aug 14, 2019

Uh oh!

candrews commented Aug 14, 2019

Uh oh!

DrahtBot commented Aug 24, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

luke-jr left a comment

Choose a reason for hiding this comment

Uh oh!

luke-jr Sep 20, 2019

Choose a reason for hiding this comment

Uh oh!

luke-jr Sep 20, 2019

Choose a reason for hiding this comment

Uh oh!

luke-jr Sep 20, 2019

Choose a reason for hiding this comment

Uh oh!

luke-jr commented Sep 20, 2019

Uh oh!

candrews commented Sep 20, 2019

Uh oh!

maflcko commented Oct 11, 2019

Uh oh!

laanwj commented Feb 5, 2020

Uh oh!

adamjonas commented May 18, 2020

Uh oh!

candrews commented May 18, 2020

Uh oh!

luke-jr commented May 18, 2020

Uh oh!

maflcko commented May 18, 2020

Uh oh!

maflcko commented May 18, 2020

Uh oh!

maflcko commented May 18, 2020

Uh oh!

maflcko commented May 31, 2020

Uh oh!

maflcko commented May 31, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

candrews commented Aug 7, 2019 •

edited by maflcko

Loading

kallewoof commented Aug 7, 2019 •

edited

Loading

DrahtBot commented Aug 24, 2019 •

edited

Loading