Skip to content

lint: convert lint-tests.sh to python #24815

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
Merged
merged 1 commit into from
Apr 25, 2022
Merged

lint: convert lint-tests.sh to python #24815

merged 1 commit into from
Apr 25, 2022

Conversation

KevinMusgrave
Copy link
Contributor

Reference issue: #24783

@KevinMusgrave KevinMusgrave changed the title Converted lint-tests.sh to python lint: converted lint-tests.sh to python Apr 9, 2022
@KevinMusgrave KevinMusgrave force-pushed the lint-tests-python-port branch from a0f5d5e to e60d9ee Compare April 9, 2022 22:22
@KevinMusgrave KevinMusgrave changed the title lint: converted lint-tests.sh to python lint: convert lint-tests.sh to python Apr 9, 2022
@DrahtBot DrahtBot added the Tests label Apr 9, 2022
@KevinMusgrave
Copy link
Contributor Author

Maybe I should replace grep, cut, sort, and uniq with python functions

@KevinMusgrave KevinMusgrave force-pushed the lint-tests-python-port branch 2 times, most recently from 74506be to 25ea0a3 Compare April 11, 2022 22:17
@KevinMusgrave
Copy link
Contributor Author

Ok I converted those functions to python, so the only non-python call is git grep.

@KevinMusgrave KevinMusgrave force-pushed the lint-tests-python-port branch 2 times, most recently from 292a1d8 to 285ee4b Compare April 11, 2022 22:38
vincenzopalazzo
vincenzopalazzo reviewed Apr 11, 2022
View reviewed changes
Copy link

@vincenzopalazzo vincenzopalazzo left a comment

Choose a reason for hiding this comment

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

utACK 285ee4b

@KevinMusgrave KevinMusgrave force-pushed the lint-tests-python-port branch from 285ee4b to b412505 Compare April 13, 2022 14:12
@KevinMusgrave
Copy link
Contributor Author

Below are example outputs, before and after the change. The only difference is that the new version adds a newline after the "matching names" test, which I think improves readability.

Before (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/banman_tests.cpp:BOOST_FIXTURE_TEST_SUITE(addrman_tests, BasicTestingSetup)
src/test/bip32_tests.cpp:BOOST_FIXTURE_TEST_SUITE(base58_tests, BasicTestingSetup)
src/test/blockchain_tests.cpp:BOOST_FIXTURE_TEST_SUITE(base58_tests, BasicTestingSetup)
src/test/blockencodings_tests.cpp:BOOST_FIXTURE_TEST_SUITE(base58_tests, RegTestingSetup)
Test suite names must be unique. The following test suite names
appear to be used more than once:

addrman_tests
base58_tests

After (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/banman_tests.cpp:BOOST_FIXTURE_TEST_SUITE(addrman_tests, BasicTestingSetup)
src/test/bip32_tests.cpp:BOOST_FIXTURE_TEST_SUITE(base58_tests, BasicTestingSetup)
src/test/blockchain_tests.cpp:BOOST_FIXTURE_TEST_SUITE(base58_tests, BasicTestingSetup)
src/test/blockencodings_tests.cpp:BOOST_FIXTURE_TEST_SUITE(base58_tests, RegTestingSetup)

Test suite names must be unique. The following test suite names
appear to be used more than once:

addrman_tests
base58_tests

laanwj
laanwj reviewed Apr 13, 2022
View reviewed changes
@KevinMusgrave KevinMusgrave force-pushed the lint-tests-python-port branch 2 times, most recently from b132506 to 21b967a Compare April 13, 2022 18:19
Eunoia1729
Eunoia1729 reviewed Apr 13, 2022
View reviewed changes
Copy link
Contributor

@Eunoia1729 Eunoia1729 left a comment

Choose a reason for hiding this comment

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

utACk
Not sure but is it a good practise to leave links in the code ?

@KevinMusgrave
Copy link
Contributor Author

When I grab a snippet from stackoverflow, I like to leave a link to credit the author but also in case I want to see the original answer again.

maflcko
maflcko reviewed Apr 15, 2022
View reviewed changes
maflcko
maflcko reviewed Apr 15, 2022
View reviewed changes
@KevinMusgrave
Converted lint-tests.sh to python
ae0e06a 
Use raw string

Use re.search instead of grep in check_matching_test_names

Replaced bash commands in check_unique_test_names with python commands

Use set and sort output

Use set comprehension

Use .splitlines()

Call grep_boost_fixture_test_suite once

splitlines() once

Fixed copyright date

Use check_output() instead of run()

add encoding='utf8'

Use clearer code for getting duplicates
@KevinMusgrave KevinMusgrave force-pushed the lint-tests-python-port branch from 21b967a to ae0e06a Compare April 15, 2022 14:11
@KevinMusgrave KevinMusgrave mentioned this pull request Apr 18, 2022
Closed
@laanwj laanwj mentioned this pull request Apr 18, 2022
Closed
25 tasks
@laanwj
Copy link
Member

laanwj commented Apr 25, 2022

Tested ACK ae0e06a

@laanwj laanwj merged commit 9eedbe9 into bitcoin:master Apr 25, 2022
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Apr 26, 2022
@laanwj
Merge bitcoin#24815: lint: convert lint-tests.sh to python
ea3961c 
ae0e06a Converted lint-tests.sh to python (TakeshiMusgrave)

Pull request description:

  Reference issue: bitcoin#24783

ACKs for top commit:
  laanwj:
    Tested ACK ae0e06a

Tree-SHA512: a118295b5b6b5199b52d46b54de871d88dd544112e7dd5001a9575d65b093af0aea390f9ad223462a4fc6a201bd8c4debe5e26bfa4860a90c97cfe300477c04a
@bitcoin bitcoin locked and limited conversation to collaborators Apr 25, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@laanwj laanwj laanwj left review comments

+3 more reviewers

@maflcko maflcko maflcko left review comments

@vincenzopalazzo vincenzopalazzo vincenzopalazzo left review comments

@Eunoia1729 Eunoia1729 Eunoia1729 left review comments

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
Tests
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@KevinMusgrave @laanwj @maflcko @vincenzopalazzo @Eunoia1729 @DrahtBot