Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

test: Always define the raii_event_tests test suite #16564

Open
wants to merge 1 commit into
base: master
from

Conversation

@candrews
Copy link

commented Aug 7, 2019

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

@candrews candrews referenced this pull request Aug 7, 2019
@kallewoof

This comment has been minimized.

Copy link
Member

commented Aug 7, 2019

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 candrews force-pushed the candrews:patch-1 branch 2 times, most recently from 2728557 to b2c8450 Aug 7, 2019
@candrews candrews changed the base branch from 0.18 to master Aug 7, 2019
@candrews

This comment has been minimized.

Copy link
Author

commented Aug 7, 2019

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

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

@MarcoFalke MarcoFalke changed the title Always define the raii_event_tests test suite test: Always define the raii_event_tests test suite Aug 7, 2019
@MarcoFalke

This comment has been minimized.

Copy link
Member

commented Aug 7, 2019

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

@candrews

This comment has been minimized.

Copy link
Author

commented Aug 7, 2019

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.

@DrahtBot DrahtBot added the Tests label Aug 7, 2019
@luke-jr

This comment has been minimized.

Copy link
Member

commented Aug 8, 2019

See also #16228

@MarcoFalke

This comment has been minimized.

Copy link
Member

commented Aug 8, 2019

Could you move the #if guard inside the boost_auto_test_case, so that the suite and test case appear only once in the file? Also, this does not compile.

@laanwj

This comment has been minimized.

Copy link
Member

commented Aug 14, 2019

Agree that this is an issue and needs to be fixed, however I don't think running empty tests is the right fix to allow for conditional tests.

@candrews

This comment has been minimized.

Copy link
Author

commented Aug 14, 2019

I agree - I much prefer the approach taken in #16228 - perhaps that can be merged?

@DrahtBot

This comment has been minimized.

Copy link
Contributor

commented Aug 24, 2019

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

No conflicts as of last run.

Copy link
Member

left a comment

Concept ACK

#ifndef EVENT_SET_MEM_FUNCTIONS_IMPLEMENTED
BOOST_AUTO_TEST_CASE(raii_event_creation)
{
// dummy; do nothing

This comment has been minimized.

Copy link
@luke-jr

luke-jr Sep 20, 2019

Member

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");

BOOST_FIXTURE_TEST_SUITE(raii_event_tests, BasicTestingSetup)

#ifndef EVENT_SET_MEM_FUNCTIONS_IMPLEMENTED

This comment has been minimized.

Copy link
@luke-jr

luke-jr Sep 20, 2019

Member

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)

BOOST_FIXTURE_TEST_SUITE(raii_event_tests, BasicTestingSetup)

#ifndef EVENT_SET_MEM_FUNCTIONS_IMPLEMENTED
BOOST_AUTO_TEST_CASE(raii_event_creation)

This comment has been minimized.

Copy link
@luke-jr

luke-jr Sep 20, 2019

Member

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 #9493
@luke-jr

This comment has been minimized.

Copy link
Member

commented Sep 20, 2019

@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

This comment has been minimized.

Copy link
Author

commented Sep 20, 2019

@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!

@candrews candrews force-pushed the candrews:patch-1 branch from b2c8450 to 9a19c9a Sep 20, 2019
@MarcoFalke

This comment has been minimized.

Copy link
Member

commented Oct 11, 2019

re-run ci

@MarcoFalke MarcoFalke closed this Oct 11, 2019
@MarcoFalke MarcoFalke reopened this Oct 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.