test: add functional test for -startupnotify #23532

brunoerg · 2021-11-17T00:58:11Z

This PR adds a functional test for -startupnotify. It basically starts the node passing a command on -startupnotify to create a file on tmp and then, we check if the file has been successfully created.

lsilva01 · 2021-11-17T02:08:54Z

Concept ACK.

Perhaps the test with valid command can create a file in test or tmp dir and checks if this file was created..

DrahtBot · 2021-11-17T04:29:31Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#22776 (rpc/wallet: add optional transaction(s) to getbalances by kallewoof)
#22751 (rpc/wallet: add simulaterawtransaction RPC by kallewoof)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

brunoerg · 2021-11-17T11:22:45Z

I've tried different valid commands with -startupnotify like: echo (common command for Unix system and Windows), tried a simple ls for Unix and dir for Windows and other ones, but the CI is failing for all of them. Any thoughts about it, @MarcoFalke?

maflcko · 2021-11-17T11:29:26Z

You can find the error in the log:

 node0 stderr Error: The experimental syscall sandbox feature (-sandbox=<mode>) is incompatible with -startupnotify (which uses execve).

brunoerg · 2021-11-17T11:53:11Z

You can find the error in the log:

 node0 stderr Error: The experimental syscall sandbox feature (-sandbox=<mode>) is incompatible with -startupnotify (which uses execve).

Sorry, I read that but didn't notice this part of the log. Thanks.

theStack · 2021-11-17T13:28:53Z

Concept ACK

brunoerg · 2021-11-17T17:31:58Z

Force-pushed with a update on expected_stderr in stop_node. I just edited the description.

brunoerg · 2021-11-18T18:35:56Z

Win64 has failed:

I think the problem may be the breaklines. Going to work to fix it.

brunoerg · 2021-11-19T10:51:08Z

Force-pushed adding os.linesep into expected windows message.

test/functional/feature_startupnotify.py

luke-jr

Ideally, a test for -startupnotify should ensure that the command is executed exactly once, and when it is executed, the node is fully started (which may involve races which make it difficult to deterministically fail).

test/functional/feature_startupnotify.py

brunoerg · 2021-12-03T12:50:02Z

Force-pushed addressing the reviews, thanks everyone for them. I just changed the approach for this test (updated the description).

jonatack

ACK 31931a5ab009256bf583025939e6e3ca2f875d9a

A couple of ideas, feel free to ignore/pick/choose:

Begin by testing the absence of tmpdir without -startupnotify before asserting on its presence with the option
Maybe abstract the magic strings to constants

 from test_framework.test_framework import BitcoinTestFramework
 
+NODE_DIR = "node0"
+FILE_NAME = "test_123_xyz"
 
 class StartupNotifyTest(BitcoinTestFramework):
     def set_test_params(self):
         self.num_nodes = 1
         self.disable_syscall_sandbox = True
-        self.extra_args = [["-startupnotify=echo 'test' >> node0/test.txt"]]
 
     def run_test(self):
-        tmpdir = os.path.join(self.options.tmpdir, "node0", "test.txt")
+        tmpdir = os.path.join(self.options.tmpdir, NODE_DIR, FILE_NAME)
+        assert not os.path.exists(tmpdir)
 
         self.log.info("When node starts, check if test.txt exists")
+        self.restart_node(0, extra_args=[f"-startupnotify=echo '{FILE_NAME}' >> {NODE_DIR}/{FILE_NAME}"])
         assert os.path.exists(tmpdir)

test/functional/feature_startupnotify.py

jonatack · 2021-12-07T20:24:35Z

Ideally, a test for -startupnotify should ensure that the command is executed exactly once, and when it is executed, the node is fully started (which may involve races which make it difficult to deterministically fail).

Maybe checking the expected result of an RPC (like getblockcount) could verify the node is fully up.

brunoerg · 2021-12-07T21:46:51Z

Force-pushed addressing @luke-jr and @jonatack tips. Thanks for the reviews.

jonatack · 2021-12-08T12:51:24Z

ACK, modulo unrelated code that appears to have crept into the last push?

brunoerg · 2021-12-10T15:57:27Z

ACK, modulo unrelated code that appears to have crept into the last push?

omg, didn't understand why it happened, going to check it.

brunoerg · 2021-12-21T17:57:47Z

fixed commits! @jonatack

Davidson-Souza · 2021-12-22T20:02:15Z

tested ACK 98014c5

OS: Arch Linux 64 bits (Linux 5.12.19-hardened1-1-hardened x86_64)
System: AMD Ryzen 5 64 bits + AMD VEGA 8 with 12GB RAM
Build: Compiled from source with no additional ./configure options

Tests:
Ran the test alone, it worked!

Also, all the tests with test/functional/test_runner.py, again everything is working just fine!

test/functional/feature_startupnotify.py

brunoerg · 2021-12-23T17:17:21Z

force-pushed addressing @luke-jr and @kristapsk reviews (check if the command is executed once).

kristapsk

ACK dbdcc783e39794d54c2f17040d248539b1ca46b0

lsilva01

reACK dbdcc78

Nice addition to test coverage.

maflcko · 2021-12-24T09:29:17Z

feature_startupnotify.py                           | ✖ Failed  | 2 s

brunoerg · 2021-12-24T13:57:57Z

feature_startupnotify.py                           | ✖ Failed  | 2 s

Fixed!

theStack

Tested ACK 1268532
(run on OpenBSD 7.0)

kristapsk

re-ACK 1268532

1268532 test: add functional test for -startupnotify (Bruno Garcia) Pull request description: This PR adds a functional test for -startupnotify. It basically starts the node passing a command on -startupnotify to create a file on tmp and then, we check if the file has been successfully created. ACKs for top commit: theStack: Tested ACK 1268532 kristapsk: re-ACK 1268532 Tree-SHA512: 5bf3e46124ee5c9d609c9993e6465d5a71a8d2275dcf07c8ce0549f013f7f8863d483b46b7164152f566468a689371ccb87f01cf118c3c9cac5b2be673b61a5c

seaona · 2022-01-27T16:25:07Z

Concept and approach ACK. Tested and working fine :)
I saw that startupnotify uses execve and I was wondering if that could be checked on the test, somehow like: in case of using sandbox (if defined(USE_SYSCALL_SANDBOX), startupnotify will not throw?

fanquake added the Tests label Nov 17, 2021

brunoerg force-pushed the 2021-11-test-startupnotify branch from c31930c to a25e636 Compare November 17, 2021 10:28

DrahtBot mentioned this pull request Nov 17, 2021

rpc/wallet: add optional transaction(s) to getbalances #22776

Closed

brunoerg force-pushed the 2021-11-test-startupnotify branch from a25e636 to 36b9623 Compare November 17, 2021 10:57

DrahtBot mentioned this pull request Nov 17, 2021

rpc/wallet: add simulaterawtransaction RPC #22751

Merged

brunoerg force-pushed the 2021-11-test-startupnotify branch 2 times, most recently from 5f7d252 to b9908fa Compare November 17, 2021 13:21

brunoerg force-pushed the 2021-11-test-startupnotify branch 3 times, most recently from 7818542 to 9e94a23 Compare November 18, 2021 14:08

brunoerg force-pushed the 2021-11-test-startupnotify branch 2 times, most recently from 510919d to e9ebe3b Compare November 18, 2021 23:53

stratospher reviewed Nov 20, 2021

View reviewed changes

test/functional/feature_startupnotify.py Outdated Show resolved Hide resolved

maflcko reviewed Nov 21, 2021

View reviewed changes

test/functional/feature_startupnotify.py Outdated Show resolved Hide resolved

brunoerg force-pushed the 2021-11-test-startupnotify branch 4 times, most recently from 7f462b1 to b0fc9f9 Compare November 24, 2021 12:59

luke-jr suggested changes Nov 30, 2021

View reviewed changes

test/functional/feature_startupnotify.py Outdated Show resolved Hide resolved

brunoerg force-pushed the 2021-11-test-startupnotify branch from b0fc9f9 to 31931a5 Compare December 3, 2021 12:46

jonatack reviewed Dec 7, 2021

View reviewed changes

test/functional/feature_startupnotify.py Outdated Show resolved Hide resolved

brunoerg force-pushed the 2021-11-test-startupnotify branch 2 times, most recently from 506c9ce to 3e2be6f Compare December 7, 2021 21:42

brunoerg force-pushed the 2021-11-test-startupnotify branch from 3e2be6f to 98014c5 Compare December 21, 2021 17:57

kristapsk reviewed Dec 22, 2021

View reviewed changes

test/functional/feature_startupnotify.py Outdated Show resolved Hide resolved

test/functional/feature_startupnotify.py Outdated Show resolved Hide resolved

brunoerg force-pushed the 2021-11-test-startupnotify branch from 98014c5 to dbdcc78 Compare December 23, 2021 17:15

kristapsk approved these changes Dec 23, 2021

View reviewed changes

lsilva01 approved these changes Dec 23, 2021

View reviewed changes

test: add functional test for -startupnotify

1268532

brunoerg force-pushed the 2021-11-test-startupnotify branch from dbdcc78 to 1268532 Compare December 24, 2021 11:42

theStack approved these changes Dec 31, 2021

View reviewed changes

kristapsk approved these changes Dec 31, 2021

View reviewed changes

maflcko merged commit 31f385c into bitcoin:master Jan 3, 2022

bitcoin locked and limited conversation to collaborators Jan 27, 2023

test: add functional test for -startupnotify #23532

test: add functional test for -startupnotify #23532

Uh oh!

Conversation

brunoerg commented Nov 17, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lsilva01 commented Nov 17, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Nov 17, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

brunoerg commented Nov 17, 2021

Uh oh!

maflcko commented Nov 17, 2021

Uh oh!

brunoerg commented Nov 17, 2021

Uh oh!

theStack commented Nov 17, 2021

Uh oh!

brunoerg commented Nov 17, 2021

Uh oh!

brunoerg commented Nov 18, 2021

Uh oh!

brunoerg commented Nov 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

luke-jr left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

brunoerg commented Dec 3, 2021

Uh oh!

jonatack left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jonatack commented Dec 7, 2021

Uh oh!

brunoerg commented Dec 7, 2021

Uh oh!

jonatack commented Dec 8, 2021

Uh oh!

brunoerg commented Dec 10, 2021

Uh oh!

brunoerg commented Dec 21, 2021

Uh oh!

Davidson-Souza commented Dec 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

brunoerg commented Dec 23, 2021

Uh oh!

kristapsk left a comment

Choose a reason for hiding this comment

Uh oh!

lsilva01 left a comment

Choose a reason for hiding this comment

Uh oh!

maflcko commented Dec 24, 2021

Uh oh!

brunoerg commented Dec 24, 2021

Uh oh!

theStack left a comment

Choose a reason for hiding this comment

Uh oh!

kristapsk left a comment

Choose a reason for hiding this comment

Uh oh!

seaona commented Jan 27, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

brunoerg commented Nov 17, 2021 •

edited

Loading

lsilva01 commented Nov 17, 2021 •

edited

Loading

DrahtBot commented Nov 17, 2021 •

edited

Loading

brunoerg commented Nov 19, 2021 •

edited

Loading

Davidson-Souza commented Dec 22, 2021 •

edited

Loading