tests: Add note about test suite naming convention in developer-notes.md #12719

practicalswift commented at 7:46 PM on March 18, 2018: contributor

Changes:

Add note about test suite naming convention
Fix exceptions
Add regression test

Rationale:

Consistent naming of test suites makes programmatic test running of specific tests/subsets of tests easier
Explicit is better than implicit

Before this commit:

$ contrib/devtools/lint-tests.sh
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/blockchain_tests.cpp:BOOST_FIXTURE_TEST_SUITE(blockchain_difficulty_tests, BasicTestingSetup)
src/test/prevector_tests.cpp:BOOST_FIXTURE_TEST_SUITE(PrevectorTests, TestingSetup)
src/wallet/test/coinselector_tests.cpp:BOOST_FIXTURE_TEST_SUITE(coin_selection_tests, WalletTestingSetup)
src/wallet/test/crypto_tests.cpp:BOOST_FIXTURE_TEST_SUITE(wallet_crypto, BasicTestingSetup)
$

After this commit:

$ contrib/devtools/lint-tests.sh
$

practicalswift renamed this:
~~tests: Add note about test suite naming convention~~
tests: Add note about test suite naming convention in developer-notes.md
on Mar 18, 2018

practicalswift force-pushed on Mar 18, 2018

meshcollider added the label Tests on Mar 18, 2018

randolf approved

tests: Add note about test suite naming convention 7b4a296a71

tests: Rename test suits not following the test suite naming convention

The name of the fixture test suite in `src/test/foo_tests.cpp`
should be `foo_tests`.

5fd864fe8a

tests: Add lint-tests.sh which checks the test suite naming convention db983beba6

practicalswift force-pushed on Mar 19, 2018

in contrib/devtools/lint-tests.sh:10 in db983beba6

   5 | +# file COPYING or http://www.opensource.org/licenses/mit-license.php.
   6 | +#
   7 | +# Check the test suite naming convention
   8 | +
   9 | +NAMING_INCONSISTENCIES=$(git grep -E '^BOOST_FIXTURE_TEST_SUITE\(' -- \
  10 | +    "src/test/**.cpp" "src/wallet/test/**.cpp" | \

conscott commented at 12:38 PM on March 20, 2018:

There is also a violation in

src/univalue/test/object.cpp:BOOST_FIXTURE_TEST_SUITE(univalue_tests, BasicTestingSetup)

but I realize that is outside the scope of test_bitcoin tests.

Could make changes upstream to univalue as well, but that's minor.

practicalswift commented at 2:34 PM on March 20, 2018:

Yes, upstreams' tests are intentionally not checked by lint-tests.sh since we don't want Travis to block on things where we need help from upstreams. All violations should be possible to fix in-tree :-)

conscott commented at 2:37 PM on March 20, 2018: contributor

Tested ACK db983beba6fec910722bacc0d67c7ae98ba566fa

eklitzke commented at 7:38 AM on March 21, 2018: contributor

Since this fixes all violations of the rule we should also add this as a required lint check in Travis.

in contrib/devtools/lint-tests.sh:12 in db983beba6

   7 | +# Check the test suite naming convention
   8 | +
   9 | +NAMING_INCONSISTENCIES=$(git grep -E '^BOOST_FIXTURE_TEST_SUITE\(' -- \
  10 | +    "src/test/**.cpp" "src/wallet/test/**.cpp" | \
  11 | +    grep -vE '/(.*?)\.cpp:BOOST_FIXTURE_TEST_SUITE\(\1, .*\)$')
  12 | +if [[ ${NAMING_INCONSISTENCIES} != "" ]]; then

eklitzke commented at 7:43 AM on March 21, 2018:

There is a serious shell quoting problem here. This isn't just a nit, it changes the semantics of the test. Use test -n and quote the variable, e.g.

if [ -n "${NAMING_INCONSISTENCIES}" ]; then
    ...

eklitzke commented at 7:57 AM on March 21, 2018:

Actually I have to take this back, apparently [[ doesn't perform word-splitting. Still scares me though.

practicalswift commented at 8:33 AM on March 21, 2018:

No need to be scared – [[ ${FOO} != "" ]] is perfectly safe AFAIK :-)

FWIW shellcheck agrees :-)

laanwj commented at 9:36 PM on March 29, 2018:

Isn't that a bash-ism, which means we'll get, as usual, at least three iterations of PRs 'fixing' this for different shells on different platforms?

I'm also no use in reviewing shell scripts. Would be happier with a python script. On the other hand this is by far not the most scary one :)

Concept ACK anyhow.

practicalswift commented at 8:20 PM on April 2, 2018:

This is a bash script and is explicit about it (#!/bin/bash), so no need for follow-up PR:s for other shells :-)

eklitzke changes_requested

eklitzke approved

eklitzke commented at 7:58 AM on March 21, 2018: contributor

tested ACK db983beba, but would like to see this added to the travis tests

practicalswift commented at 8:27 AM on March 21, 2018: contributor

@eklitzke Scripts matching contrib/devtools/lint-*.sh are automatically run by Travis, so no need to add it explicitly :-)

MarcoFalke commented at 11:31 AM on March 21, 2018: member

utACK db983beba6fec910722bacc0d67c7ae98ba566fa

MarcoFalke merged this on Apr 1, 2018

MarcoFalke closed this on Apr 1, 2018

MarcoFalke referenced this in commit 9beded5860 on Apr 1, 2018

PastaPastaPasta referenced this in commit ddf2c3b511 on Jun 9, 2020

PastaPastaPasta referenced this in commit 7184453c83 on Jun 9, 2020

PastaPastaPasta referenced this in commit b3c10bfdf5 on Jun 10, 2020

PastaPastaPasta referenced this in commit 6a8a4a406a on Jun 10, 2020

PastaPastaPasta referenced this in commit 4a4ead32aa on Jun 11, 2020

PastaPastaPasta referenced this in commit a92a20b2f9 on Jun 11, 2020

PastaPastaPasta referenced this in commit 9241f08110 on Jun 11, 2020

PastaPastaPasta referenced this in commit c1c84d6987 on Jun 12, 2020

practicalswift deleted the branch on Apr 10, 2021

gades referenced this in commit 31c42248c5 on Mar 8, 2022

DrahtBot locked this on Aug 18, 2022