Skip to content

lint: fix lint-tests.py failing on correct names #26725

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

lint: fix lint-tests.py failing on correct names #26725

wants to merge 1 commit into from

Conversation

willcl-ark
Copy link
Member

lint-tests.py is currently failing to detect that filenames are correct and therefore returning incorrect result and error code from the script. This fixes the matching.

Ouput before:

❯ test/lint/lint-tests.py
The test suite in file src/test/foo_tests.cpp should be named
"foo_tests". Please make sure the following test suites follow
that convention:

src/test/addrman_tests.cpp:61:BOOST_FIXTURE_TEST_SUITE(addrman_tests, BasicTestingSetup)
src/test/argsman_tests.cpp:22:BOOST_FIXTURE_TEST_SUITE(argsman_tests, BasicTestingSetup)
src/test/banman_tests.cpp:16:BOOST_FIXTURE_TEST_SUITE(banman_tests, BasicTestingSetup)
src/test/base58_tests.cpp:21:BOOST_FIXTURE_TEST_SUITE(base58_tests, BasicTestingSetup)
src/test/bip32_tests.cpp:160:BOOST_FIXTURE_TEST_SUITE(bip32_tests, BasicTestingSetup)
...
etc.

Output after (all tests ok):

❯ test/lint/lint-tests.py; echo $status
0

Output after (with an intentially renamed test):

❯ git mv src/test/addrman_tests.cpp src/test/addrman_tests_incorrect.cpp

will@ubuntu in bitcoin on  lint-name-test [$»] via 🐍 v3.6.15
❯ test/lint/lint-tests.py; and $status
The test suite in file src/test/foo_tests.cpp should be named
"foo_tests". Please make sure the following test suites follow
that convention:

src/test/addrman_tests_incorrect.cpp

1

@willcl-ark
lint: fix lint-tests.py failing on correct names
7536ff2 
lint-tests.py is currently failing to detect that filenames are correct
and therefore returning incorrect result and error code from the script.
@DrahtBot
Copy link
Contributor

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

Reviews

See the guideline for information on the review process.
A summary of reviews will appear here.

@willcl-ark
Copy link
Member Author

With this, #26721 and #26257 the usefulness of linter would be greatly improved.

@maflcko
Copy link
Member

maflcko commented Dec 19, 2022

CI is green on master, so it is unclear to me what this is fixing.

@willcl-ark
Copy link
Member Author

Closing because:

  1. My fix is incorrect for the intended test case.
  2. It seems you are correct and that this is a local issue with my setup in some way, although I cannot figure out why

    bitcoin/test/lint/lint-tests.py

    Lines 30 to 34 in 65f5cfd

    not_matching = [
    x
    for x in test_suite_list
    if re.search(r"/(.*?)\.cpp:BOOST_FIXTURE_TEST_SUITE\(\1, .*\)", x) is None
    ]
    does not succeed in its regex matching locally for me, which I can see that it does on CI.

I am running with python 3.6.15 as per .python-version and have packages matching CI versions installed:

❯ pip freeze
codespell==2.2.1
flake8==4.0.1
importlib-metadata==4.2.0
mccabe==0.6.1
mypy==0.942
mypy-extensions==0.4.3
pycodestyle==2.8.0
pyflakes==2.4.0
pyzmq==22.3.0
toml==0.10.2
tomli==1.2.3
typed-ast==1.5.4
typing_extensions==4.1.1
vulture==2.3
zipp==3.6.0

vs

bitcoin/ci/lint/04_install.sh

Lines 19 to 23 in 65f5cfd

${CI_RETRY_EXE} pip3 install codespell==2.2.1
${CI_RETRY_EXE} pip3 install flake8==4.0.1
${CI_RETRY_EXE} pip3 install mypy==0.942
${CI_RETRY_EXE} pip3 install pyzmq==22.3.0
${CI_RETRY_EXE} pip3 install vulture==2.3

When I run lint-tests.py on master I see the following output which I don't see in CI:

will@ubuntu in bitcoin on  master [$⇡] via 🐍 v3.6.15
❯ ./test/lint/lint-tests.py
The test suite in file src/test/foo_tests.cpp should be named
"foo_tests". Please make sure the following test suites follow
that convention:

src/test/addrman_tests.cpp:61:BOOST_FIXTURE_TEST_SUITE(addrman_tests, BasicTestingSetup)
src/test/argsman_tests.cpp:22:BOOST_FIXTURE_TEST_SUITE(argsman_tests, BasicTestingSetup)
src/test/banman_tests.cpp:16:BOOST_FIXTURE_TEST_SUITE(banman_tests, BasicTestingSetup)
src/test/base58_tests.cpp:21:BOOST_FIXTURE_TEST_SUITE(base58_tests, BasicTestingSetup)
src/test/bip32_tests.cpp:160:BOOST_FIXTURE_TEST_SUITE(bip32_tests, BasicTestingSetup)
src/test/blockchain_tests.cpp:50:BOOST_FIXTURE_TEST_SUITE(blockchain_tests, BasicTestingSetup)
src/test/blockencodings_tests.cpp:18:BOOST_FIXTURE_TEST_SUITE(blockencodings_tests, RegTestingSetup)
src/test/blockmanager_tests.cpp:17:BOOST_FIXTURE_TEST_SUITE(blockmanager_tests, BasicTestingSetup)
src/test/bloom_tests.cpp:24:BOOST_FIXTURE_TEST_SUITE(bloom_tests, BasicTestingSetup)
src/test/checkqueue_tests.cpp:35:BOOST_FIXTURE_TEST_SUITE(checkqueue_tests, NoLockLoggingTestingSetup)
src/test/coins_tests.cpp:99:BOOST_FIXTURE_TEST_SUITE(coins_tests, BasicTestingSetup)
src/test/compress_tests.cpp:25:BOOST_FIXTURE_TEST_SUITE(compress_tests, BasicTestingSetup)
src/test/crypto_tests.cpp:27:BOOST_FIXTURE_TEST_SUITE(crypto_tests, BasicTestingSetup)
src/test/dbwrapper_tests.cpp:23:BOOST_FIXTURE_TEST_SUITE(dbwrapper_tests, BasicTestingSetup)
src/test/denialofservice_tests.cpp:36:BOOST_FIXTURE_TEST_SUITE(denialofservice_tests, TestingSetup)
src/test/descriptor_tests.cpp:371:BOOST_FIXTURE_TEST_SUITE(descriptor_tests, BasicTestingSetup)
src/test/flatfile_tests.cpp:13:BOOST_FIXTURE_TEST_SUITE(flatfile_tests, BasicTestingSetup)
src/test/fs_tests.cpp:16:BOOST_FIXTURE_TEST_SUITE(fs_tests, BasicTestingSetup)
src/test/getarg_tests.cpp:18:BOOST_FIXTURE_TEST_SUITE(getarg_tests, BasicTestingSetup)
src/test/headers_sync_chainwork_tests.cpp:58:BOOST_FIXTURE_TEST_SUITE(headers_sync_chainwork_tests, HeadersGeneratorSetup)
src/test/httpserver_tests.cpp:10:BOOST_FIXTURE_TEST_SUITE(httpserver_tests, BasicTestingSetup)
src/test/i2p_tests.cpp:19:BOOST_FIXTURE_TEST_SUITE(i2p_tests, BasicTestingSetup)
src/test/interfaces_tests.cpp:16:BOOST_FIXTURE_TEST_SUITE(interfaces_tests, TestChain100Setup)
src/test/key_io_tests.cpp:20:BOOST_FIXTURE_TEST_SUITE(key_io_tests, BasicTestingSetup)
src/test/key_tests.cpp:32:BOOST_FIXTURE_TEST_SUITE(key_tests, BasicTestingSetup)
src/test/logging_tests.cpp:20:BOOST_FIXTURE_TEST_SUITE(logging_tests, BasicTestingSetup)
src/test/mempool_tests.cpp:16:BOOST_FIXTURE_TEST_SUITE(mempool_tests, TestingSetup)
src/test/merkle_tests.cpp:10:BOOST_FIXTURE_TEST_SUITE(merkle_tests, TestingSetup)
src/test/miner_tests.cpp:54:BOOST_FIXTURE_TEST_SUITE(miner_tests, MinerTestingSetup)
src/test/miniscript_tests.cpp:159:BOOST_FIXTURE_TEST_SUITE(miniscript_tests, BasicTestingSetup)
src/test/multisig_tests.cpp:19:BOOST_FIXTURE_TEST_SUITE(multisig_tests, BasicTestingSetup)
src/test/net_peer_eviction_tests.cpp:18:BOOST_FIXTURE_TEST_SUITE(net_peer_eviction_tests, BasicTestingSetup)
src/test/net_tests.cpp:36:BOOST_FIXTURE_TEST_SUITE(net_tests, RegTestingSetup)
src/test/netbase_tests.cpp:23:BOOST_FIXTURE_TEST_SUITE(netbase_tests, BasicTestingSetup)
src/test/orphanage_tests.cpp:18:BOOST_FIXTURE_TEST_SUITE(orphanage_tests, TestingSetup)
src/test/pmt_tests.cpp:28:BOOST_FIXTURE_TEST_SUITE(pmt_tests, BasicTestingSetup)
src/test/policyestimator_tests.cpp:16:BOOST_FIXTURE_TEST_SUITE(policyestimator_tests, ChainTestingSetup)
src/test/pow_tests.cpp:12:BOOST_FIXTURE_TEST_SUITE(pow_tests, BasicTestingSetup)
src/test/prevector_tests.cpp:16:BOOST_FIXTURE_TEST_SUITE(prevector_tests, TestingSetup)
src/test/raii_event_tests.cpp:16:BOOST_FIXTURE_TEST_SUITE(raii_event_tests, BasicTestingSetup)
src/test/random_tests.cpp:14:BOOST_FIXTURE_TEST_SUITE(random_tests, BasicTestingSetup)
src/test/rbf_tests.cpp:17:BOOST_FIXTURE_TEST_SUITE(rbf_tests, TestingSetup)
src/test/rest_tests.cpp:12:BOOST_FIXTURE_TEST_SUITE(rest_tests, BasicTestingSetup)
src/test/rpc_tests.cpp:83:BOOST_FIXTURE_TEST_SUITE(rpc_tests, RPCTestingSetup)
src/test/sanity_tests.cpp:11:BOOST_FIXTURE_TEST_SUITE(sanity_tests, BasicTestingSetup)
src/test/script_p2sh_tests.cpp:51:BOOST_FIXTURE_TEST_SUITE(script_p2sh_tests, BasicTestingSetup)
src/test/script_segwit_tests.cpp:10:BOOST_FIXTURE_TEST_SUITE(script_segwit_tests, BasicTestingSetup)
src/test/script_standard_tests.cpp:20:BOOST_FIXTURE_TEST_SUITE(script_standard_tests, BasicTestingSetup)
src/test/script_tests.cpp:126:BOOST_FIXTURE_TEST_SUITE(script_tests, BasicTestingSetup)
src/test/scriptnum_tests.cpp:13:BOOST_FIXTURE_TEST_SUITE(scriptnum_tests, BasicTestingSetup)
src/test/serfloat_tests.cpp:16:BOOST_FIXTURE_TEST_SUITE(serfloat_tests, BasicTestingSetup)
src/test/serialize_tests.cpp:15:BOOST_FIXTURE_TEST_SUITE(serialize_tests, BasicTestingSetup)
src/test/settings_tests.cpp:50:BOOST_FIXTURE_TEST_SUITE(settings_tests, BasicTestingSetup)
src/test/sighash_tests.cpp:118:BOOST_FIXTURE_TEST_SUITE(sighash_tests, BasicTestingSetup)
src/test/sigopcount_tests.cpp:27:BOOST_FIXTURE_TEST_SUITE(sigopcount_tests, BasicTestingSetup)
src/test/skiplist_tests.cpp:14:BOOST_FIXTURE_TEST_SUITE(skiplist_tests, BasicTestingSetup)
src/test/sock_tests.cpp:18:BOOST_FIXTURE_TEST_SUITE(sock_tests, BasicTestingSetup)
src/test/streams_tests.cpp:13:BOOST_FIXTURE_TEST_SUITE(streams_tests, BasicTestingSetup)
src/test/system_tests.cpp:24:BOOST_FIXTURE_TEST_SUITE(system_tests, BasicTestingSetup)
src/test/timedata_tests.cpp:19:BOOST_FIXTURE_TEST_SUITE(timedata_tests, BasicTestingSetup)
src/test/transaction_tests.cpp:188:BOOST_FIXTURE_TEST_SUITE(transaction_tests, BasicTestingSetup)
src/test/txreconciliation_tests.cpp:11:BOOST_FIXTURE_TEST_SUITE(txreconciliation_tests, BasicTestingSetup)
src/test/txrequest_tests.cpp:17:BOOST_FIXTURE_TEST_SUITE(txrequest_tests, BasicTestingSetup)
src/test/util_tests.cpp:54:BOOST_FIXTURE_TEST_SUITE(util_tests, BasicTestingSetup)
src/test/validation_block_tests.cpp:34:BOOST_FIXTURE_TEST_SUITE(validation_block_tests, MinerTestingSetup)
src/test/validation_chainstate_tests.cpp:19:BOOST_FIXTURE_TEST_SUITE(validation_chainstate_tests, ChainTestingSetup)
src/test/validation_chainstatemanager_tests.cpp:26:BOOST_FIXTURE_TEST_SUITE(validation_chainstatemanager_tests, ChainTestingSetup)
src/test/validation_flush_tests.cpp:11:BOOST_FIXTURE_TEST_SUITE(validation_flush_tests, TestingSetup)
src/test/validation_tests.cpp:16:BOOST_FIXTURE_TEST_SUITE(validation_tests, TestingSetup)
src/test/validationinterface_tests.cpp:13:BOOST_FIXTURE_TEST_SUITE(validationinterface_tests, TestingSetup)
src/test/versionbits_tests.cpp:185:BOOST_FIXTURE_TEST_SUITE(versionbits_tests, BasicTestingSetup)
src/wallet/test/availablecoins_tests.cpp:14:BOOST_FIXTURE_TEST_SUITE(availablecoins_tests, WalletTestingSetup)
src/wallet/test/coinselector_tests.cpp:23:BOOST_FIXTURE_TEST_SUITE(coinselector_tests, WalletTestingSetup)
src/wallet/test/db_tests.cpp:16:BOOST_FIXTURE_TEST_SUITE(db_tests, BasicTestingSetup)
src/wallet/test/feebumper_tests.cpp:18:BOOST_FIXTURE_TEST_SUITE(feebumper_tests, WalletTestingSetup)
src/wallet/test/init_tests.cpp:14:BOOST_FIXTURE_TEST_SUITE(init_tests, InitWalletDirTestingSetup)
src/wallet/test/ismine_tests.cpp:18:BOOST_FIXTURE_TEST_SUITE(ismine_tests, BasicTestingSetup)
src/wallet/test/psbt_wallet_tests.cpp:15:BOOST_FIXTURE_TEST_SUITE(psbt_wallet_tests, WalletTestingSetup)
src/wallet/test/scriptpubkeyman_tests.cpp:14:BOOST_FIXTURE_TEST_SUITE(scriptpubkeyman_tests, BasicTestingSetup)
src/wallet/test/spend_tests.cpp:16:BOOST_FIXTURE_TEST_SUITE(spend_tests, WalletTestingSetup)
src/wallet/test/wallet_crypto_tests.cpp:14:BOOST_FIXTURE_TEST_SUITE(wallet_crypto_tests, BasicTestingSetup)
src/wallet/test/wallet_tests.cpp:44:BOOST_FIXTURE_TEST_SUITE(wallet_tests, WalletTestingSetup)
src/wallet/test/wallet_transaction_tests.cpp:12:BOOST_FIXTURE_TEST_SUITE(wallet_transaction_tests, WalletTestingSetup)
src/wallet/test/walletdb_tests.cpp:13:BOOST_FIXTURE_TEST_SUITE(walletdb_tests, BasicTestingSetup)

If I can't figure out why, I guess I'll re-open as an issue.

@willcl-ark willcl-ark closed this Dec 19, 2022
@willcl-ark
Copy link
Member Author

Certainly seems to be something wrong with my local regex searching via python, although I'm not really sure how that can happen.

For example modifying the regex line to use a different regex (with look behind capture group) will have it pass locally for me, as it seems to be doing on CI:

diff --git a/test/lint/lint-tests.py b/test/lint/lint-tests.py
index 849ddcb96..cafba9f45 100755
--- a/test/lint/lint-tests.py
+++ b/test/lint/lint-tests.py
@@ -30,7 +30,7 @@ def check_matching_test_names(test_suite_list):
     not_matching = [
         x
         for x in test_suite_list
-        if re.search(r"/(.*?)\.cpp:BOOST_FIXTURE_TEST_SUITE\(\1, .*\)", x) is None
+        if re.search(r"/((?<=\/)[^\/]*)\.cpp:\d*:BOOST_FIXTURE_TEST_SUITE\(\1, .*\)", x) is None
     ]
     if len(not_matching) > 0:
         not_matching = "\n".join(not_matching)

@maflcko
Copy link
Member

maflcko commented Dec 19, 2022

To debug this, we'd need steps to reproduce. Ideally, starting from a fresh install of your OS

@willcl-ark
Copy link
Member Author

Ok I've found the issue in case anyone else ever ends up here. my git grep was returning linenumbers by default and throwing off the regex.

@bitcoin bitcoin locked and limited conversation to collaborators Dec 19, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@willcl-ark @DrahtBot @maflcko