qa: Functional test improvements #30463

pull hebasto wants to merge 3 commits into bitcoin:master from hebasto:240716-ftest changing 247 files +254 −253
  1. hebasto commented at 8:14 pm on July 16, 2024: member

    This PR includes changes split from #30454. They improve the functional test framework, allowing users to run individual functional tests from the build directory in the new CMake-based build system.

    This functionality is not available for out-of-source builds using the current Autotools-based build system, which always requires write permissions for the source directory. Nevertheless, this PR can be tested as suggested in #30463 (comment):

    1. Make an out-of-source build:
    0$ ./autogen.sh
1$ mkdir ../build && cd ../build
2$ ../bitcoin/configure
3$ make
    1. Create a symlink in the build directory to a functional test:
    0$ ln --symbolic ../../../bitcoin/test/functional/wallet_disable.py ./test/functional/
    1. Run this symlink:
    0$ ./test/functional/wallet_disable.py

    The last command fails on the master branch:

    0Traceback (most recent call last):
1  File "/home/hebasto/git/build/./test/functional/wallet_disable.py", line 31, in <module>
2    DisableWalletTest().main()
3    ^^^^^^^^^^^^^^^^^^^
4  File "/home/hebasto/git/bitcoin/test/functional/test_framework/test_framework.py", line 106, in __init__
5    self.parse_args()
6  File "/home/hebasto/git/bitcoin/test/functional/test_framework/test_framework.py", line 210, in parse_args
7    config.read_file(open(self.options.configfile))
8                     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
9FileNotFoundError: [Errno 2] No such file or directory: '/home/hebasto/git/bitcoin/test/config.ini'

    and succeeds with this PR.

  2. DrahtBot commented at 8:14 pm on July 16, 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 maflcko, stickies-v, glozow

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

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #30207 (validation: Improve, document and test logic for chains building on invalid blocks by mzumsande)

    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.

  3. DrahtBot added the label Tests on Jul 16, 2024
  4. maflcko commented at 8:25 pm on July 16, 2024: member

    The first commit could be split up as a scripted-diff? Also, formatting:

     0diff --git a/test/functional/interface_http.py b/test/functional/interface_http.py
 1index 567aa33cbe..dbdceb52d1 100755
 2--- a/test/functional/interface_http.py
 3+++ b/test/functional/interface_http.py
 4@@ -109 +109 @@ if __name__ == '__main__':
 5-    HTTPBasicsTest(__file__).main ()
 6+    HTTPBasicsTest(__file__).main()
 7--
 8diff --git a/test/functional/rpc_getchaintips.py b/test/functional/rpc_getchaintips.py
 9index 65a9583c22..1bd4413ce1 100755
10--- a/test/functional/rpc_getchaintips.py
11+++ b/test/functional/rpc_getchaintips.py
12@@ -61 +61 @@ if __name__ == '__main__':
13-    GetChainTipsTest(__file__).main ()
14+    GetChainTipsTest(__file__).main()
  5. scripted-diff: Add `__file__` argument to `BitcoinTestFramework.init()` 
    -BEGIN VERIFY SCRIPT-
sed -i -e 's/\s*().main\s*()/(__file__).main()/' $(git ls-files test/functional/*.py)
sed -i -e 's/def __init__(self)/def __init__(self, test_file)/' test/functional/test_framework/test_framework.py
-END VERIFY SCRIPT-
    a0473442d1
  6. qa: Consider `cache` and `config.ini` relative to invocation directory 
    In CMake-based build system (1) `config.ini` is created in the build
directory, and (2) `cache` must also be created in the same directory.

This change enables running individual functional tests from the build
directory.
    9bf7ca6cad
  7. qa: Do not assume running `feature_asmap.py` from source directory a8e3af1a82
  8. hebasto force-pushed on Jul 16, 2024
  9. hebasto commented at 9:07 pm on July 16, 2024: member

    The first commit could be split up as a scripted-diff? Also, formatting:

     0diff --git a/test/functional/interface_http.py b/test/functional/interface_http.py
 1index 567aa33cbe..dbdceb52d1 100755
 2--- a/test/functional/interface_http.py
 3+++ b/test/functional/interface_http.py
 4@@ -109 +109 @@ if __name__ == '__main__':
 5-    HTTPBasicsTest(__file__).main ()
 6+    HTTPBasicsTest(__file__).main()
 7--
 8diff --git a/test/functional/rpc_getchaintips.py b/test/functional/rpc_getchaintips.py
 9index 65a9583c22..1bd4413ce1 100755
10--- a/test/functional/rpc_getchaintips.py
11+++ b/test/functional/rpc_getchaintips.py
12@@ -61 +61 @@ if __name__ == '__main__':
13-    GetChainTipsTest(__file__).main ()
14+    GetChainTipsTest(__file__).main()

    Thanks! Implemented.

  10. maflcko commented at 7:27 am on July 17, 2024: member

    No behaviour changes for Autotools builds.

    I don’t think this is true. This is a bugfix.

    Previously one could not run the tests in the autotools out-of-source build dir:

     0$ ./test/functional/wallet_disable.py 
 1bash: ./test/functional/wallet_disable.py: No such file or directory
 2$ ln --symbolic ../../../test/functional/wallet_disable.py ./test/functional/
 3$ ./test/functional/wallet_disable.py 
 4Traceback (most recent call last):
 5  File "./test/functional/wallet_disable.py", line 31, in <module>
 6    DisableWalletTest().main()
 7    ^^^^^^^^^^^^^^^^^^^
 8  File "$srcdir/test/functional/test_framework/test_framework.py", line 106, in __init__
 9    self.parse_args()
10  File "$srcdir/test/functional/test_framework/test_framework.py", line 210, in parse_args
11    config.read_file(open(self.options.configfile))
12                     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
13FileNotFoundError: [Errno 2] No such file or directory: '$srcdir/test/config.ini'

    After this fix, it is possible.

    Wrong: Also, the output of ./test/functional/test_runner.py --help changes, so this very much is a behavior change.

  11. maflcko commented at 7:29 am on July 17, 2024: member
    Concept ACK on the behavior changes and the bugfix, but please properly describe the changes.
  12. maflcko added this to the milestone 28.0 on Jul 17, 2024
  13. hebasto commented at 7:48 am on July 17, 2024: member

    @maflcko

    Previously one could not run the tests in the autotools out-of-source build dir:

    After this fix, it is possible.

    That’s true. But I did not consider this case as it requires a manual creation of a symlink to a test, which Autottols do not do, but CMake does.

  14. maflcko commented at 7:53 am on July 17, 2024: member
    In any case, creating the symlinks seems like a good way to test this, no?
  15. maflcko commented at 8:13 am on July 17, 2024: member

    Tested with the steps I provided.

    tested ACK a8e3af1a82dd584a1cc3ffbe587e66889f72e3c 🎨

    Signature:

    0untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
1RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
2trusted comment: tested ACK a8e3af1a82dd584a1cc3ffbe587e66889f72e3c 🎨
30l6ISxyQX7ey5ttupZ8f66fkchEymRF7a/bxNMR0+JttWLFMFvFkaYsxPISzikFFEUg17o1uHFGeuqHoN7xpBg==
  16. hebasto commented at 8:17 am on July 17, 2024: member

    In any case, creating the symlinks seems like a good way to test this, no?

    Sure thing. The PR description has been updated.

  17. hebasto commented at 8:22 am on July 17, 2024: member
    Friendly ping @stickies-v :)
  18. in test/functional/feature_asmap.py:137 in a8e3af1a82 
    132@@ -133,7 +133,8 @@ def run_test(self):
133         self.node = self.nodes[0]
134         self.datadir = self.node.chain_path
135         self.default_asmap = os.path.join(self.datadir, DEFAULT_ASMAP_FILENAME)
136-        self.asmap_raw = os.path.join(os.path.dirname(os.path.realpath(__file__)), ASMAP)
137+        base_dir = self.config["environment"]["SRCDIR"]
138+        self.asmap_raw = os.path.join(base_dir, ASMAP)
    maflcko commented at 8:30 am on July 17, 2024:
    question: Is there a reason why this needs to change, but the other os.path.dirname(os.path.realpath(__file__)) and Path(__file__).parents are fine?
    hebasto commented at 8:52 am on July 17, 2024:
    Other cases point to files that reside in the build directory. Or do you have any particular example?
    maflcko commented at 9:41 am on July 17, 2024:

    Yeah, but this one uses os.path.realpath(__file__) as well (so should be in the srcdir already), no?

    With my steps to reproduce, it passes before and after this commit.

    Do you have other steps to reproduce?

    hebasto commented at 10:40 am on July 17, 2024:
    Can our previous conversation be helpful?
    maflcko commented at 11:36 am on July 17, 2024:

    Maybe, but it would be good to clarify what of the previous conversation is relevant.

    Are you saying that this is needed for Windows and can not be tested on Unix?

    hebasto commented at 1:30 pm on July 17, 2024:

    Please disregard my previous comments if they contradict this one. My apologies for any confusion.

    On Windows, creating symbolic links requires a permission, which is disabled by default. Therefore, CMake creates hard links (or copies as a fallback). In that case, os.path.realpath(__file__) will point at a file in the build directory.

    An example of an error can be observed here:

     0268/313 - feature_asmap.py failed, Duration: 2 s
 1stdout:
 22024-07-17T11:26:07.749000Z TestFramework (INFO): PRNG seed is: 7216823379970217686
 32024-07-17T11:26:07.780000Z TestFramework (INFO): Initializing test directory D:\a\_temp\test_runner_₿_🏃_20240717_110914\feature_asmap_42
 42024-07-17T11:26:08.584000Z TestFramework (INFO): Test bitcoind with no -asmap arg passed
 52024-07-17T11:26:09.525000Z TestFramework (INFO): Test bitcoind -asmap=<absolute path>
 62024-07-17T11:26:09.629000Z TestFramework (ERROR): Unexpected exception caught during testing
 7Traceback (most recent call last):
 8  File "D:\a\bitcoin\bitcoin\build\test\functional\test_framework\test_framework.py", line 132, in main
 9    self.run_test()
10  File "D:\a\bitcoin\bitcoin\build\test\functional\feature_asmap.py", line 139, in run_test
11    self.test_asmap_with_absolute_path()
12  File "D:\a\bitcoin\bitcoin\build\test\functional\feature_asmap.py", line 61, in test_asmap_with_absolute_path
13    shutil.copyfile(self.asmap_raw, filename)
14  File "C:\hostedtoolcache\windows\Python\3.12.4\x64\Lib\shutil.py", line 260, in copyfile
15    with open(src, 'rb') as fsrc:
16         ^^^^^^^^^^^^^^^
17FileNotFoundError: [Errno 2] No such file or directory: 'D:\\a\\bitcoin\\bitcoin\\build\\test\\functional\\../../src/test/data/asmap.raw'
    maflcko commented at 11:44 am on July 18, 2024:

    Ok, so it can be reproduced in Linux with hard links.

    However, this leads back to my original question: Why not change the other places as well?

    For me, they also fail:

     0$ ln    $PWD/../test/functional/p2p_dos_header_tree.py   ./test/functional/
 1$ ./test/functional/p2p_dos_header_tree.py 
 22024-07-18T11:42:08.340000Z TestFramework (INFO): PRNG seed is: 5276922920789009906
 32024-07-18T11:42:08.341000Z TestFramework (INFO): Initializing test directory /tmp/bitcoin_func_test_9ppp3fwq
 42024-07-18T11:42:08.741000Z TestFramework (INFO): Read headers data
 52024-07-18T11:42:08.742000Z TestFramework (ERROR): Unexpected exception caught during testing
 6Traceback (most recent call last):
 7  File "$build_dir/test/functional/test_framework/test_framework.py", line 132, in main
 8    self.run_test()
 9  File "$build_dir/./test/functional/p2p_dos_header_tree.py", line 37, in run_test
10    with open(self.headers_file_path, encoding='utf-8') as headers_data:
11         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
12FileNotFoundError: [Errno 2] No such file or directory: '$build_dir/test/functional/data/blockheader_testnet3.hex'
    hebasto commented at 9:04 pm on July 18, 2024:

    Ok, so it can be reproduced in Linux with hard links.

    Right. But we do not use hard links on non-Windows systems.

    However, this leads back to my original question: Why not change the other places as well?

    In other places, os.path.realpath(__file__) is used in conjunction with relative paths in the test/functional/ directory. Soft links (on non-Windows systems) and hard links (on Windows) are created for every file in the test/functional/ directory recursively. Therefore, the created paths are valid.

    feature_asmap.py is an exception because data/asmap.raw resides outside of the test/functional/ directory.

    As an alternative, the data/asmap.raw file can be moved to somewhere within the test/functional/ directory.

    maflcko commented at 8:11 am on July 19, 2024:

    In other places, os.path.realpath(__file__) is used in conjunction with relative paths in the test/functional/ directory.

    Thank you for the explanation. It makes sense, given that cmake creates all links recursively in this dir.

    However, this leads back to my original question: Why not change the other affected places?

    This still fails for me (and I expect it to fail in the cmake branch as well):

     0$ ln     $PWD/../test/functional/interface_rest.py    ./test/functional/
 1$ ./test/functional/interface_rest.py --valgrind 
 22024-07-19T08:07:39.275000Z TestFramework (ERROR): Assertion failed
 3Traceback (most recent call last):
 4  File "$bld/test/functional/test_framework/test_framework.py", line 569, in start_nodes
 5    node.wait_for_rpc_connection()
 6  File "$bld/test/functional/test_framework/test_node.py", line 270, in wait_for_rpc_connection
 7    raise FailedToStartError(self._node_msg(
 8test_framework.test_node.FailedToStartError: [node 0] bitcoind exited with status 1 during initialization.
 9==831680== FATAL: can't open suppressions file "$bld/contrib/valgrind.supp"
10************************
    hebasto commented at 10:59 am on July 19, 2024:

    Your example creates a hard link on Linux, which is not what happens in the CMake-based build system.

    Do you think we should adjust our functional tests in way to handle hard links on Linux?

    maflcko commented at 11:06 am on July 19, 2024:
    Oh, I see. You are saying that valgrind is not supported on Windows and hard links in the test framework are not supported on Linux, so there is nothing to be done here?
    maflcko commented at 11:11 am on July 19, 2024:

    (Resolving discussion for now. Thanks for explaining and sorry for the lengthy replies.)

    Edit: I don’t have the resolve permission, but yeah, this is resolved from my side.

  19. stickies-v approved
  20. stickies-v commented at 11:20 am on July 17, 2024: contributor

    ACK a8e3af1a82dd584a1cc3ffbe587e66889f72e3c7

    I’m able to run individual tests from an out-of-source build with the instructions provided. I tried running a bunch of other tests than wallet_disable too, including all the ones that reference __file__ and those seem to work fine too.

  21. maflcko commented at 8:22 am on July 22, 2024: member
    Discussions are resolved and this seems rfm with two reviews?
  22. glozow commented at 11:08 am on July 22, 2024: member
    ACK a8e3af1a82dd584a1cc3ffbe587e66889f72e3c7, tested with the steps in op
  23. glozow merged this on Jul 22, 2024
  24. glozow closed this on Jul 22, 2024
  25. hebasto deleted the branch on Jul 22, 2024


hebasto DrahtBot maflcko stickies-v glozow

Labels
Tests

Milestone
28.0

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-10-18 04:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me