Skip to content

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

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

Merged
merged 3 commits into from
Apr 1, 2018
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
7b4a296
tests: Add note about test suite naming convention
practicalswift Mar 18, 2018
5fd864f
tests: Rename test suits not following the test suite naming convention
practicalswift Mar 18, 2018
db983be
tests: Add lint-tests.sh which checks the test suite naming convention
practicalswift Mar 18, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 19 additions & 0 deletions contrib/devtools/lint-tests.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
#!/bin/bash
#
# Copyright (c) 2018 The Bitcoin Core developers
# Distributed under the MIT software license, see the accompanying
# file COPYING or http://www.opensource.org/licenses/mit-license.php.
#
# Check the test suite naming convention

NAMING_INCONSISTENCIES=$(git grep -E '^BOOST_FIXTURE_TEST_SUITE\(' -- \
"src/test/**.cpp" "src/wallet/test/**.cpp" | \
Copy link
Contributor

@conscott conscott Mar 20, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 :-)

grep -vE '/(.*?)\.cpp:BOOST_FIXTURE_TEST_SUITE\(\1, .*\)$')
if [[ ${NAMING_INCONSISTENCIES} != "" ]]; then
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
    ...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@practicalswift practicalswift Mar 21, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

FWIW shellcheck agrees :-)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

echo "The test suite in file src/test/foo_tests.cpp should be named"
echo "\"foo_tests\". Please make sure the following test suites follow"
echo "that convention:"
echo
echo "${NAMING_INCONSISTENCIES}"
exit 1
fi
2 changes: 2 additions & 0 deletions doc/developer-notes.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,8 @@ code.
- Constant names are all uppercase, and use `_` to separate words.
- Class names, function names and method names are UpperCamelCase
(PascalCase). Do not prefix class names with `C`.
- Test suite naming convention: The Boost test suite in file
`src/test/foo_tests.cpp` should be named `foo_tests`.

- **Miscellaneous**
- `++i` is preferred over `i++`.
Expand Down
2 changes: 1 addition & 1 deletion src/test/blockchain_tests.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ void TestDifficulty(uint32_t nbits, double expected_difficulty)
RejectDifficultyMismatch(difficulty, expected_difficulty);
}

BOOST_FIXTURE_TEST_SUITE(blockchain_difficulty_tests, BasicTestingSetup)
BOOST_FIXTURE_TEST_SUITE(blockchain_tests, BasicTestingSetup)

BOOST_AUTO_TEST_CASE(get_difficulty_for_very_low_target)
{
Expand Down
2 changes: 1 addition & 1 deletion src/test/prevector_tests.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@

#include <boost/test/unit_test.hpp>

BOOST_FIXTURE_TEST_SUITE(PrevectorTests, TestingSetup)
BOOST_FIXTURE_TEST_SUITE(prevector_tests, TestingSetup)

template<unsigned int N, typename T>
class prevector_tester {
Expand Down
4 changes: 2 additions & 2 deletions src/wallet/crypter.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,15 +67,15 @@ class CMasterKey

typedef std::vector<unsigned char, secure_allocator<unsigned char> > CKeyingMaterial;

namespace wallet_crypto
namespace crypto_tests
{
class TestCrypter;
}

/** Encryption/decryption context with key information */
class CCrypter
{
friend class wallet_crypto::TestCrypter; // for test access to chKey/chIV
friend class crypto_tests::TestCrypter; // for test access to chKey/chIV
private:
std::vector<unsigned char, secure_allocator<unsigned char>> vchKey;
std::vector<unsigned char, secure_allocator<unsigned char>> vchIV;
Expand Down
2 changes: 1 addition & 1 deletion src/wallet/test/coinselector_tests.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@
#include <boost/test/unit_test.hpp>
#include <random>

BOOST_FIXTURE_TEST_SUITE(coin_selection_tests, WalletTestingSetup)
BOOST_FIXTURE_TEST_SUITE(coinselector_tests, WalletTestingSetup)

// how many times to run all the tests to have a chance to catch errors that only show up with particular random shuffles
#define RUN_TESTS 100
Expand Down
2 changes: 1 addition & 1 deletion src/wallet/test/crypto_tests.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@

#include <boost/test/unit_test.hpp>

BOOST_FIXTURE_TEST_SUITE(wallet_crypto, BasicTestingSetup)
BOOST_FIXTURE_TEST_SUITE(crypto_tests, BasicTestingSetup)

class TestCrypter
{
Expand Down