Skip to content
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.

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

test: Refactor subtree exclusion in lint tests #24435

Closed

Conversation

maxraustin
Copy link

refs #17413.

There are several commonly excluded directories in the lint tests. Rather than having these subtrees be copied in each of these test files, they can be pulled from a shared list so that any future changes to these exclusions can be isolated to one location rather than spread out among many files.

This first commit is a proof of concept demonstrating how this can be applied. I saw that we currently have a list of ignored words in a text file used for the spelling lint test, so I followed that general idea and moved the list of ignored subtrees into a text file as well. This can be read and parsed into whatever format we want. The extended-lint-cppcheck.sh and lint-includes.sh tests use a regex with just these directories, but some of the examples in the linked issue have additional unique exclusions so the solution needs to be able to support that.

I wanted to create an early WIP pull request with a proof of concept to present the idea for discussion. There are several other areas that this list of subtrees can be used, outlined in the ref'd issue.

refs bitcoin#17413 Rather than having the commonly excluded subtrees be copied in each of these tests, they can be pulled into a shared list so that any future changes to these exclusions does not require as many file changes. This first commit is a proof of concept demonstrating how this can be applied.
@DrahtBot DrahtBot added the Tests label Feb 24, 2022
@laanwj
Copy link
Member

laanwj commented Feb 24, 2022

Concept ACK, thanks for working on this.

@shaavan
Copy link
Contributor

shaavan commented Feb 24, 2022

Concept ACK

I see there are quite a few places where these subtrees are individually excluded. Putting them into a singular list will make the excluded files easier to manage and the code cleaner.

@injust90
Copy link

Concept ACK

… changes

refs bitcoin#17413 This commit changes the remaining lint tests to use the consolidated list of excluded subtrees. I updated the changes I made to the original tests to more closely match the regular expressions I was using in the rest of the lint scripts as well.
@maxraustin maxraustin changed the title [WIP] test: Refactor subtree exclusion in lint tests test: Refactor subtree exclusion in lint tests Mar 24, 2022
@maxraustin
Copy link
Author

maxraustin commented Mar 24, 2022

The remaining lint tests have been updated, this should be ready for more review now.

Edit: There seems to still be an issue with the whitespace test, looking into that one still.

@DrahtBot
Copy link
Contributor

DrahtBot commented Apr 5, 2022

🐙 This pull request conflicts with the target branch and needs rebase.

Want to unsubscribe from rebase notifications on this pull request? Just convert this pull request to a "draft".

@DrahtBot
Copy link
Contributor

DrahtBot commented Apr 5, 2022

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #24766 (lint: convert spellchecking lint test to python by fjahr)
  • #24762 (lint: Start to use py lint scripts by MarcoFalke)

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.

@laanwj
Copy link
Member

laanwj commented May 9, 2022

Needs update for Python ports and removal of extended-lint-cppcheck.sh.

@fanquake
Copy link
Member

@maxraustin are you still workign on this?

@achow101
Copy link
Member

Closing this as it has not had any activity in a while. If you are interested in continuing work on this, please leave a comment so that it can be reopened.

@achow101 achow101 closed this Oct 12, 2022
@bitcoin bitcoin locked and limited conversation to collaborators Oct 21, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants