doc: cmake: prepend "build" to functional/test

LarryRuane commented at 4:08 am on September 10, 2024: contributor

This is a small follow-on to #30741, prepend build/ to the path for test_runner.py.

DrahtBot commented at 4:08 am on September 10, 2024: contributor

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

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	jonatack, tdb3, maflcko
Concept ACK	furszy
Stale ACK	kevkevinpal, pablomartin4btc

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

DrahtBot added the label Docs on Sep 10, 2024

LarryRuane force-pushed on Sep 10, 2024

LarryRuane commented at 4:21 am on September 10, 2024: contributor

This patch would have helped me; I’m new to cmake; after building it for the first time, I tried to run the functional tests (the way I was used to), and it failed in a strange way:

0$ test/functional/test_runner.py 
1Traceback (most recent call last):
2  File "/sd/g/bitcoin/test/functional/test_runner.py", line 950, in <module>
3    main()
4  File "/sd/g/bitcoin/test/functional/test_runner.py", line 465, in main
5    config.read_file(open(configfile, encoding="utf8"))
6                     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
7FileNotFoundError: [Errno 2] No such file or directory: '/sd/g/bitcoin/test/functional/../config.ini'

Aware that we’re now using cmake, I checked the top-level README, and it looked like I was running the test correctly:

These tests can be run (if the test dependencies are installed) with: test/functional/test_runner.py

This was pretty confusing for a while. I finally figured out that I need to run the test script from the build directory. This PR updates the documentation (in the two places I found after a fairly careful search).

I also thought it would be helpful to improve the error message in case someone else does this. With this PR, it looks like this:

0$ test/functional/test_runner.py 
1Traceback (most recent call last):
2  File "/sd/g/bitcoin/test/functional/test_runner.py", line 952, in <module>
3    main()
4  File "/sd/g/bitcoin/test/functional/test_runner.py", line 466, in main
5    raise Exception(f"config.ini file {configfile} not found, be sure to run the this script from within the build directory")
6Exception: config.ini file /sd/g/bitcoin/test/functional/../config.ini not found, be sure to run the this script from within the build directory

in README.md:57 in 15cc65649b outdated

52@@ -53,7 +53,8 @@ and extending unit tests can be found in [/src/test/README.md](/src/test/README.
53 
54 There are also [regression and integration tests](/test), written
55 in Python.
56-These tests can be run (if the [test dependencies](/test) are installed) with: `test/functional/test_runner.py`
57+These tests can be run (if the [test dependencies](/test) are installed) with: `build/test/functional/test_runner.py`.
58+Do not run the (identical) program `test/functional/test_runner.py` (without the build directory).

tdb3 commented at 4:40 am on September 10, 2024:

This extra line seems like overkill if the proceeding line is being updated to include “build”

jonatack commented at 4:01 pm on September 13, 2024:

Not sure if needed, but maybe good to use:

(if the test dependencies are installed and assuming the build directory is named build)

This extra line seems like overkill if the proceeding line is being updated to include “build”

Agree

in test/functional/test_framework/test_framework.py:209 in 15cc65649b outdated

205@@ -206,6 +206,8 @@ def parse_args(self, test_file):
206         self.options.timeout_factor = self.options.timeout_factor or (4 if self.options.valgrind else 1)
207         self.options.previous_releases_path = previous_releases_path
208 
209+        if not os.path.isfile(self.options.configfile):

tdb3 commented at 4:43 am on September 10, 2024:

Seems like it might be enough to update the instructions rather than change test_framework.py or test_runner.py

fanquake commented at 4:08 pm on October 8, 2024:

Think I agree with this as well.

LarryRuane commented at 5:32 pm on October 8, 2024:

Thanks, I don’t mind removing the code changes at all (I was on the fence on this part of the change), should we wait a little while to see if there are other opinions?

fanquake commented at 8:19 pm on October 8, 2024:

I think it’d be good to just get this finished so the docs are updated. So that we don’t end up with even more PRs to fix the same docs.

tdb3 commented at 4:45 am on September 10, 2024: contributor

Thanks for noticing this. I’d support the documentation change without changing the python programs.

kevkevinpal commented at 12:28 pm on September 10, 2024: contributor

tested ACK 15cc656

I’m fine with the extra log added to test/functional/test_framework/test_framework.py and test/functional/test_runner.py

One nit is that the new error logs are the exact same and defined in two different areas of the code but not any thing that would make me not want this merged

in test/functional/README.md:13 in 15cc65649b outdated

 9@@ -10,7 +10,7 @@ that file and modify to fit your needs.
10 
11 #### Coverage
12 
13-Running `test/functional/test_runner.py` with the `--coverage` argument tracks which RPCs are
14+Running `build/test/functional/test_runner.py` with the `--coverage` argument tracks which RPCs are

jonatack commented at 3:58 pm on September 13, 2024:

Maybe begin this sentence with something like “Assuming the build directory is named build, running…”

jonatack commented at 1:40 pm on October 10, 2024:

Per feedback in #30859 (review), begin this sentence with “Assuming the build directory is named build, running…”

I agreed with the feedback not to add it where it was already mentioned higher up in the same file, but in this file there is no other mention.

in test/functional/test_runner.py:466 in 15cc65649b outdated

461@@ -462,6 +462,8 @@ def main():
462     # Read config generated by configure.
463     config = configparser.ConfigParser()
464     configfile = os.path.abspath(os.path.dirname(__file__)) + "/../config.ini"
465+    if not os.path.isfile(configfile):
466+        raise Exception(f"config.ini file {configfile} not found, be sure to run the this script from within the build directory")

jonatack commented at 4:28 pm on September 13, 2024:

0        raise Exception(f"config.ini file {configfile} not found, be sure to run this script from within the build directory")

in test/functional/test_framework/test_framework.py:210 in 15cc65649b outdated

205@@ -206,6 +206,8 @@ def parse_args(self, test_file):
206         self.options.timeout_factor = self.options.timeout_factor or (4 if self.options.valgrind else 1)
207         self.options.previous_releases_path = previous_releases_path
208 
209+        if not os.path.isfile(self.options.configfile):
210+            raise Exception(f"config.ini file {self.options.configfile} not found, be sure to run the this script from within the build directory")

jonatack commented at 4:28 pm on September 13, 2024:

0            raise Exception(f"config.ini file {self.options.configfile} not found, be sure to run this script from within the build directory")

jonatack commented at 4:30 pm on September 13, 2024: member

Concept ACK. I came across this as well, and the new error message would indeed be helpful. (Bonus points if the message can be defined in one place, didn’t check.)

furszy commented at 8:05 pm on September 13, 2024: member

Concept ACK. I keep calling the src test files by inertia..

pablomartin4btc commented at 11:02 am on September 16, 2024: member

ACK 15cc65649b78ffcbd1e1b302c2e4866623cee294

(Bonus points if the message can be defined in one place, didn’t check.)

I agree. ConfigParser should not raise an exception if the config file path is invalid?

DrahtBot requested review from furszy on Sep 16, 2024

DrahtBot requested review from jonatack on Sep 16, 2024

jonatack commented at 2:30 pm on September 16, 2024: member

(DrahtBot is pinging for a re-review, but the pull hasn’t been updated yet.)

maflcko commented at 3:10 pm on September 16, 2024: member

(DrahtBot is pinging for a re-review, but the pull hasn’t been updated yet.)

This is intentional, because at least one reviewer thinks that no further update is needed and indicated with their review that the change would be fine to merge. The Bot can’t evaluate the content of the review comments itself (obviously), so it is up to each reviewer to evaluate whether it can be merged as-is, or not.

jonatack commented at 3:24 pm on September 16, 2024: member

Thanks @maflcko (hm, maybe bump the threshold to 2 new ACKs, or drop the rule in this case).

Friendly ping to @LarryRuane to update.

maflcko commented at 3:34 pm on September 16, 2024: member

Thanks @maflcko (hm, maybe bump the threshold to 2 new ACKs, or drop the rule in this case).

Pull requests, or issues are welcome, see also https://github.com/maflcko/DrahtBot/issues/33. However, in this case I’d find it useful to be pinged (otherwise I may miss/forget to re-review or otherwise reply).

LarryRuane force-pushed on Sep 17, 2024

DrahtBot added the label CI failed on Sep 17, 2024

DrahtBot commented at 6:44 am on September 17, 2024: contributor

🚧 At least one of the CI tasks failed. Debug: https://github.com/bitcoin/bitcoin/runs/30239893248

Make sure to run all tests locally, according to the documentation.

The failure may happen due to a number of reasons, for example:

Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.

Leave a comment here, if you need help tracking down a confusing failure.

in src/test/README.md:23 in 96f4820f1f outdated

18@@ -19,7 +19,8 @@ and tests weren't explicitly disabled.
19 Assuming the build directory is named `build`, the unit tests can be run
20 with `ctest --test-dir build`, which includes unit tests from subtrees.
21 
22-To run the unit tests manually, launch `build/src/test/test_bitcoin`. To recompile
23+To run the unit tests manually, launch `build/src/test/test_bitcoin`
24+(assuming `build` is your build directory). To recompile

fanquake commented at 10:12 am on September 20, 2024:

You’re adding (assuming build is your build directory) here, but 3 lines up it already says Assuming the build directory is named build,.

in src/test/README.md:33 in 96f4820f1f outdated

29@@ -29,6 +30,7 @@ To add more unit tests, add `BOOST_AUTO_TEST_CASE` functions to the existing
30 implement new `BOOST_AUTO_TEST_SUITE` sections.
31 
32 To run the GUI unit tests manually, launch `build/src/qt/test/test_bitcoin-qt`
33+(assuming `build` is your build directory).

fanquake commented at 10:12 am on September 20, 2024:

Same here. I don’t think we need to duplicate (assuming build is your build directory) 10 lines apart. Especially when all the build commands here used build as the build dir.

in src/crc32c/README.md:89 in 96f4820f1f outdated

84@@ -85,7 +85,8 @@ cmake -DCRC32C_BUILD_TESTS=0 -DCRC32C_BUILD_BENCHMARKS=0 .. && make all install
85 
86 ## Development
87 
88-The following command (when executed from `build/`) (re)builds the project and
89+The following command (when executed from within your build directory,
90+for example,`build/`) (re)builds the project and

fanquake commented at 10:12 am on September 20, 2024:

You can’t modify the subtree here.

in doc/developer-notes.md:344 in 96f4820f1f outdated

339@@ -340,7 +340,8 @@ Recommendations:
340 
341 Assuming the build directory is named `build`,
342 the documentation can be generated with `cmake --build build --target docs`.
343-The resulting files will be located in `build/doc/doxygen/html`;
344+The resulting files will be located in `build/doc/doxygen/html`
345+(assuming `build` is your build directory);

fanquake commented at 10:13 am on September 20, 2024:

2 lines above, this already says Assuming the build directory is named build,

LarryRuane commented at 4:29 pm on September 20, 2024: contributor

@fanquake - thanks, I’ll fix all these this weekend.

LarryRuane force-pushed on Sep 21, 2024

DrahtBot removed the label CI failed on Sep 21, 2024

LarryRuane force-pushed on Oct 9, 2024

maflcko commented at 4:14 pm on October 9, 2024: member

lgtm ACK c15ef096ce317ae911dd756593adeac9d9a0b5a1

DrahtBot requested review from pablomartin4btc on Oct 9, 2024

tdb3 approved

tdb3 commented at 10:45 pm on October 9, 2024: contributor

ACK c15ef096ce317ae911dd756593adeac9d9a0b5a1

DrahtBot requested review from jonatack on Oct 9, 2024

jonatack changes_requested

jonatack commented at 11:18 pm on October 9, 2024: member

After this push, ISTM that the following change is now inadvertently missing.

0--- a/test/functional/README.md
1+++ b/test/functional/README.md
2@@ -10,7 +10,8 @@ that file and modify to fit your needs.
3  
4-Running `test/functional/test_runner.py` with the `--coverage` argument tracks which RPCs are
5+Running `build/test/functional/test_runner.py` (assuming `build` is your build
6+directory) with the `--coverage` argument tracks which RPCs are
7 called by the tests and prints a report of uncovered RPCs in the summary. This

DrahtBot requested review from jonatack on Oct 9, 2024

jonatack changes_requested

jonatack commented at 11:21 pm on October 9, 2024: member

After this push, ISTM that the following change is now inadvertently missing.

0--- a/test/functional/README.md
1+++ b/test/functional/README.md
2@@ -10,7 +10,8 @@ that file and modify to fit your needs.
3  
4-Running `test/functional/test_runner.py` with the `--coverage` argument tracks which RPCs are
5+Running `build/test/functional/test_runner.py` (assuming `build` is your build
6+directory) with the `--coverage` argument tracks which RPCs are
7 called by the tests and prints a report of uncovered RPCs in the summary. This

LarryRuane force-pushed on Oct 10, 2024

LarryRuane commented at 7:16 am on October 10, 2024: contributor

Force pushed 6ab688c74b61c4403f831f1df54a350548ce8664 to restore the accidentally-dropped change that Jon pointed out.

jonatack commented at 1:41 pm on October 10, 2024: member

LGTM modulo comment

doc: cmake: prepend and explain "build/" where needed e64b2f1a16

LarryRuane force-pushed on Oct 11, 2024

LarryRuane commented at 5:26 pm on October 11, 2024: contributor

Force pushed Jon’s suggestion, thanks - #30859 (review)

jonatack commented at 5:31 pm on October 11, 2024: member

ACK e64b2f1a16e8d0ad2cd181d84e3b70312e3cdf33

DrahtBot requested review from maflcko on Oct 11, 2024

DrahtBot requested review from tdb3 on Oct 11, 2024

tdb3 approved

tdb3 commented at 0:26 am on October 12, 2024: contributor

re ACK e64b2f1a16e8d0ad2cd181d84e3b70312e3cdf33

maflcko commented at 10:06 am on October 15, 2024: member

lgtm ACK e64b2f1a16e8d0ad2cd181d84e3b70312e3cdf33

fanquake merged this on Oct 15, 2024

fanquake closed this on Oct 15, 2024

LarryRuane deleted the branch on Oct 15, 2024

doc: cmake: prepend “build” to functional/test_runner.py #30859

Code Coverage

Reviews