Before
After
124 | The tests contain logging at different levels (debug, info, warning, etc). By 125 | default: 126 | 127 | - when run through the test_runner harness, *all* logs are written to 128 | - `test_framework.log` and no logs are output to the console. 129 | + test_framework.log` and no logs are output to the console.
This change wasn't intentional? :-)
Typo, agreed :)
Indeed! Fixed.
Concept ACK
Thanks for improving the documentation!
117 | 118 | +The test framework logs to two files: 119 | + * `<test data directory>/test_framework.log` 120 | + * `<test data directory>/<node>/regtest/debug.log`. 121 | + 122 | +The `<test data directory>` path is by default set to the temporary directory of your OS, and its full path is written as the very first line of the test output. The `<node>` part identifies the relevant test node (from `node1` to `node4`).
Would be nice to stay within the max line length seen in the source file. It shouldn't change the result in markdown but the source will be easier to read.
Agreed! Fixed.
115 | 116 | ##### Test logging 117 | 118 | +The test framework logs to two files: 119 | + * `<test data directory>/test_framework.log` 120 | + * `<test data directory>/<node>/regtest/debug.log`.
Perhaps replace <node> with node<number> here and in the following sentence.
Good idea! Made it so.
Concept ACK. Filenames from root seems clearer to me too, though I don't have a strong opinion. Note that in test/functional/README there are non-root filenames as well, so if consensus is in that direction why not update both.
Note that in test/functional/README there are non-root filenames as well, so if consensus is in that direction why not update both.
Good find. I'll try do make it happen, but I'd rather do it in another PR. Keeping things small.
118 | +The test framework logs to two files: 119 | + * `<test data directory>/test_framework.log` 120 | + * `<test data directory>/node<number>/regtest/debug.log`. 121 | + 122 | +The `<test data directory>` path is by default set to the temporary directory of 123 | +your OS, and its full path is written as the very first line of the test output.
It is not of the OS, but rather of the python environment. And it can be overwritten via a parameter to the test (runner)
Suggestion: "its full path is provided in the first line of the test output."
Well, the python tempfile module is used to achieve cross-platform support, but the current phrasing "...is by default set to the temporary directory of your OS" is still valid, IMO.
Regarding the overwriting ability: My thinking was that adding "by default" would suggest to the reader that there are other options (which are mentioned elsewhere). Brevity is nice but I could add more info if you think it is needed.
Suggestion: "its full path is provided in the first line of the test output."
Agreed, better! Fixed.
Yeah, no strong opinion. Just for reference: https://docs.python.org/3.8/library/tempfile.html#tempfile.gettempdir
ACK, but I agree that both files should be fixed up. I think 12 lines to review is small enough to include more changes.
119 | + * `<test data directory>/test_framework.log` 120 | + * `<test data directory>/node<number>/regtest/debug.log`. 121 | + 122 | +The `<test data directory>` path is by default set to the temporary directory of 123 | +your OS, and its full path is written as the very first line of the test output. 124 | +The `node<number>` part identifies the relevant test node (from `node1` to`node4`).
Add a space after "to" here: to`node4`
ACK, but I agree that both files should be fixed up. I think 12 lines to review is small enough to include more changes. @MarcoFalke Fair enough. I'll look into it!
Note that in test/functional/README there are non-root filenames as well, so if consensus is in that direction why not update both. @MarcoFalke Made some changes to test/functional/README.md, which is a bit different in character compared to test/README.md. I tried to keep the text as readable as possible while still informing the reader about the location of each file. I used both full path and linking, depending on textual context (Do we want the the reader to look at the file or issue a command in a terminal?). All links still work when read from GitHub.
117 | 118 | +The test framework logs to two files: 119 | + * `<test data directory>/test_framework.log` 120 | + * `<test data directory>/node<number>/regtest/debug.log`. 121 | + 122 | +The `<test data directory>` path is by default set to the temporary directory of
This paragraph could be shorter, and omit the OS part, along the lines of:
"The first line of the test output provides the test data directory path. The node number is the relevant test node from node1 to node4."
Brevity is key, but personally I think your suggestion would be taking it too far. IMO most readers would be interested in the OS part. Anyone else?
3 | @@ -4,13 +4,13 @@ 4 | 5 | #### Example test 6 | 7 | -The [example_test.py](example_test.py) is a heavily commented example of a test case that uses both 8 | -the RPC and P2P interfaces. If you are writing your first test, copy that file 9 | -and modify to fit your needs. 10 | +The python script [test/functional/example_test.py](example_test.py) is a heavily commented example
It's a test file. I'd suggest "The file test/functional/example_test.py is a ..." or just begin with the link.
Fixed, thanks.
81 | @@ -82,7 +82,7 @@ P2P messages. These can be found in the following source files: 82 | 83 | #### Using the P2P interface 84 | 85 | -- `messages.py` contains all the definitions for objects that pass 86 | +- [messages.py](test/functional/test_framework/messages.py) contains all the definitions for objects that pass
Are the label and link inverted here?
(the contents)
No, but the link was a bit off. Fix coming shortly.
95 | @@ -96,32 +96,33 @@ the Bitcoin Core node application logic. For custom behaviour, subclass the 96 | P2PInterface object and override the callback methods. 97 | 98 | - Can be used to write tests where specific P2P protocol behavior is tested. 99 | -Examples tests are `p2p_unrequested_blocks.py`, `p2p_compactblocks.py`. 100 | +Examples tests are [p2p_unrequested_blocks.py](p2p_unrequested_blocks.py), [p2p_compactblocks.py](p2p_compactblocks.py). 101 | 102 | -### test-framework modules 103 | +### Test framework modules 104 | +The following are some useful modules for test developers. They are all located in [test/functional/test_framework](test_framework/)
Missing punctuation at end of the line. For brevity can omit "some" and "all". Would it be better to add a slash at the end of the test/functional/test_framework/ label?
Ideally limit line lengths to 120 chars.
Thanks for your feedback. Fixed.
squashed
All feedback has been adressed, can this PR be merged?
The tests don't pass
Missed the little red X again.... Fixed some linting errors.
All feedback has been adressed, can this PR be merged?
118 | +The test framework logs to two files: 119 | + * `<test data directory>/test_framework.log` 120 | + * `<test data directory>/node<number>/regtest/debug.log`. 121 | + 122 | +The first line of the test output provides the test data directory path. The 123 | +node number is the relevant test node, from node1 to node4.
tests can set their own number of nodes and it starts at node0, btw
I decided to close this PR. Thanks for all the good feedback but I feel it grew too much. Not in number of lines but in number of comments and elapsed calendar time. If someone wants to use these ideas in another PR, please feel free to do so! (Or reopen this one.)