[WIP] Better stderr testing in functional tests #12379

jnewbery · 2018-02-07T16:03:25Z

(WIP because if we're not careful this could result in a lot of false +ves. Should be tested on different platforms to verify portability).

#12362 was only observed when running the functional tests locally because:

by defatul libc logs to /dev/tty instead of stderr
the functional tests only check for substring inclusion in stderr when we're expecting bitcoind to fail.

This PR tightens our checking of stderr and will cause tests to fail if there is any unexpected message in stderr:

commit rename TestNode to TestP2PConn in tests is not necessary in this PR, but it removes a name conflict in the functional tests (which makes it difficult to see all the places that TestNode is being called)
commit Move assert_start_raises_init_error method to TestNode moves a method from test_framework to test_node (in general, methods which only affect a single node should be methods in TestNode).
commit Write stdout/stderr to datadir instead of temp file writes stderr to a file in the datadir instead of a temporary file. This helps with debugging in the case of failure.
commit Use LIBC_FATAL_STDERR=1 in tests_ ensures that libc failures are logged to stderr instead of the terminal.
commit Update assert_start_raises_init_eror() to test for an exact m… changes assert_start_raises_init_eror() to match on an exact regex instead of a substring (with due apologies to @laanwj for test: Make ua_comment test pass on 0.16.0 #12302).
commit Assert that bitcoind stdout is empty on shutdown asserts that stderr is empty on bitcoind shutdown.

Several test scripts define a subclass of P2PInterface called TestNode. This commit renames those to TestP2PConn since we already have a TestNode class in the test framework. -BEGIN VERIFY SCRIPT- sed -i s/TestNode/TestP2PConn/ test/functional/*py test/functional/test_framework/comptool.py _END VERIFY SCRIPT-

By default, libc will print fatal errors to /dev/tty instead of stderr. Adding the LIBC_FATAL_STDERR_ to the environment variables allows us to catch libc errors in stderr and test for them.

…atch in stdout

maflcko

The scripted diff should really go into a separate pull

maflcko · 2018-02-07T22:49:54Z

test/functional/test_framework/test_node.py

+                    log_stderr.seek(0)
+                    stderr = log_stderr.read().decode('utf-8')
+                    if re.fullmatch(expected_msg + '\n', stderr) is None:
+                        raise AssertionError('Expected stdout "{}" does not match stdout:\n"{}'.format(expected_msg, stderr))


Nit: Should say stderr in the string?

laanwj · 2018-02-08T19:41:57Z

Concept ACK

randolf · 2018-02-15T02:34:42Z

Concept ACK.

Sjors · 2018-02-20T17:57:43Z

I ran test_runner.py on MacOS 10.13.3. wallet_encryption.py, wallet_multiwallet.py and wallet_multiwallet.py --usecli failed. I ran them again individually and they still failed. See logs. The first test fails on master too (see #12375), but those other two pass on master (ffc6e48).

jnewbery · 2018-03-19T19:04:42Z

I've pulled out the first commit into #12728, and the assert_start_raises_init_error() commits have been pulled into #12718.

I'll leave this PR open for now, but will close and replace with a separate PR for the libc/stdout changes once those other two PRs are merged.

…_eror (jnewbery) fae1374 qa: Allow for partial_match when checking init error (MarcoFalke) 5812273 [Tests] Require exact match in assert_start_raises_init_eror() (John Newbery) 0ec08a6 [Tests] Move assert_start_raises_init_error method to TestNode (John Newbery) Pull request description: Extracted from #12379, because the changes are important on their own. This allows for exact testing, since the match can be specified with a strict regex. Internal details (such as exact formatting of the error message) can still be fuzzed away by regex wildcards. Tree-SHA512: 605d2c9c42362a32d42321b066637577a026d0bb8cfc1c9f5737a4ca6503ffe85457a5122cea6e1101053ccc6c8aa1bbae3602e1fa7d2988bf7d5c275f412f66

maflcko · 2018-03-22T09:52:26Z

Needs rebase

jnewbery · 2018-03-22T13:53:49Z

Parts of this PR have been merged in other PRs. Closing this in favour of #12755 which contains only the parts which have not yet been merged.

beee49b [tests] Allow stderr to be tested against specified string (John Newbery) e503671 [Tests] Use LIBC_FATAL_STDERR_=1 in tests (John Newbery) c22ce8a [Tests] Write stdout/stderr to datadir instead of temp file. (John Newbery) Pull request description: **Due to a merge conflict, this is now based on #10267. Please review that PR first!** Subset of #12379 now that parts of that PR have been merged. #12362 was only observed when running the functional tests locally because: - by defatul libc logs to `/dev/tty` instead of stderr - the functional tests only check for substring inclusion in stderr when we're expecting bitcoind to fail. This PR tightens our checking of stderr and will cause tests to fail if there is any unexpected message in stderr: - commit *Write stdout/stderr to datadir instead of temp file* writes stderr to a file in the datadir instead of a temporary file. This helps with debugging in the case of failure. - commit *Use LIBC_FATAL_STDERR=1 in tests* ensures that libc failures are logged to stderr instead of the terminal. commit *Assert that bitcoind stdout is empty on shutdown* asserts that stderr is empty on bitcoind shutdown. Tree-SHA512: 21111030e667b3b686f2a7625c2b625ebcfb6998e1cccb4f3932e8b5d21fb514b19a73ac971595d049343430e9a63155986a7f5648cad55b8f36f3c58b1c7048

…es_init_eror (jnewbery) fae1374 qa: Allow for partial_match when checking init error (MarcoFalke) 5812273 [Tests] Require exact match in assert_start_raises_init_eror() (John Newbery) 0ec08a6 [Tests] Move assert_start_raises_init_error method to TestNode (John Newbery) Pull request description: Extracted from bitcoin#12379, because the changes are important on their own. This allows for exact testing, since the match can be specified with a strict regex. Internal details (such as exact formatting of the error message) can still be fuzzed away by regex wildcards. Tree-SHA512: 605d2c9c42362a32d42321b066637577a026d0bb8cfc1c9f5737a4ca6503ffe85457a5122cea6e1101053ccc6c8aa1bbae3602e1fa7d2988bf7d5c275f412f66 Signed-off-by: pasta <pasta@dashboost.org>

beee49b [tests] Allow stderr to be tested against specified string (John Newbery) e503671 [Tests] Use LIBC_FATAL_STDERR_=1 in tests (John Newbery) c22ce8a [Tests] Write stdout/stderr to datadir instead of temp file. (John Newbery) Pull request description: **Due to a merge conflict, this is now based on bitcoin#10267. Please review that PR first!** Subset of bitcoin#12379 now that parts of that PR have been merged. bitcoin#12362 was only observed when running the functional tests locally because: - by defatul libc logs to `/dev/tty` instead of stderr - the functional tests only check for substring inclusion in stderr when we're expecting bitcoind to fail. This PR tightens our checking of stderr and will cause tests to fail if there is any unexpected message in stderr: - commit *Write stdout/stderr to datadir instead of temp file* writes stderr to a file in the datadir instead of a temporary file. This helps with debugging in the case of failure. - commit *Use LIBC_FATAL_STDERR=1 in tests* ensures that libc failures are logged to stderr instead of the terminal. commit *Assert that bitcoind stdout is empty on shutdown* asserts that stderr is empty on bitcoind shutdown. Tree-SHA512: 21111030e667b3b686f2a7625c2b625ebcfb6998e1cccb4f3932e8b5d21fb514b19a73ac971595d049343430e9a63155986a7f5648cad55b8f36f3c58b1c7048

jnewbery added 5 commits February 7, 2018 09:22

[Tests] Move assert_start_raises_init_error method to TestNode

55e4c2b

[Tests] Write stdout/stderr to datadir instead of temp file.

9ce0dc4

[Tests] Use LIBC_FATAL_STDERR_=1 in tests

34e115a

By default, libc will print fatal errors to /dev/tty instead of stderr. Adding the LIBC_FATAL_STDERR_ to the environment variables allows us to catch libc errors in stderr and test for them.

[Tests] Update assert_start_raises_init_eror() to test for an exact m…

b5b917f

…atch in stdout

laanwj added the Tests label Feb 7, 2018

maflcko reviewed Feb 7, 2018

View reviewed changes

[Tests] Assert that bitcoind stderr is empty on shutdown

a13e614

jnewbery force-pushed the test_full_stderr2 branch from f70e799 to a13e614 Compare February 8, 2018 18:15

maflcko mentioned this pull request Mar 18, 2018

[Tests] Require exact match in assert_start_raises_init_eror (jnewbery) #12718

Merged

jnewbery mentioned this pull request Mar 22, 2018

[tests] Better stderr testing #12755

Merged

jnewbery closed this Mar 22, 2018

HashUnlimited mentioned this pull request Sep 6, 2018

[tests] Better stderr testing chaincoin/chaincoin#311

Merged

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[WIP] Better stderr testing in functional tests #12379

[WIP] Better stderr testing in functional tests #12379

Uh oh!

jnewbery commented Feb 7, 2018

Uh oh!

maflcko left a comment

Uh oh!

maflcko Feb 7, 2018

Uh oh!

laanwj commented Feb 8, 2018

Uh oh!

randolf commented Feb 15, 2018

Uh oh!

Sjors commented Feb 20, 2018 •

edited

Loading

Uh oh!

jnewbery commented Mar 19, 2018

Uh oh!

maflcko commented Mar 22, 2018

Uh oh!

jnewbery commented Mar 22, 2018

Uh oh!

Uh oh!

[WIP] Better stderr testing in functional tests #12379

[WIP] Better stderr testing in functional tests #12379

Uh oh!

Conversation

jnewbery commented Feb 7, 2018

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

maflcko Feb 7, 2018

Choose a reason for hiding this comment

Uh oh!

laanwj commented Feb 8, 2018

Uh oh!

randolf commented Feb 15, 2018

Uh oh!

Sjors commented Feb 20, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jnewbery commented Mar 19, 2018

Uh oh!

maflcko commented Mar 22, 2018

Uh oh!

jnewbery commented Mar 22, 2018

Uh oh!

Uh oh!

Sjors commented Feb 20, 2018 •

edited

Loading