Introduce automated validation for test format #994
Conversation
This script is intended to identify common test file formatting errors prior to their acceptance into the project. It is designed to support future extensions for additional validation rules.
There was a problem hiding this comment.
Just checking, a local lint run is returning 0 errors.
./tools/lint/lint.py --whitelist lint.whitelist test/
Linting 27502 files.
Linting complete. 0 errors found.
I left some comments on linting the license headers.
It's great to have a lint for the frontmatter.
I wonder if we should require an esid for every new and modified file, but this is a discussion for a follow up work.
| match = _LICENSE_PATTERN.search(source) | ||
|
|
||
| if not match: | ||
| return 'No license information found.' |
There was a problem hiding this comment.
I'm positive #851 is on the way to reach final consensus soon, potentially in the next meeting. So I would avoid having this check. It's good to fix the current tests, but hopefully an unnecessary check for future test files.
There was a problem hiding this comment.
How about we remove these two lines as part of gh-851?
| _MAX_YEAR = 2030 | ||
|
|
||
| _LICENSE_PATTERN = re.compile( | ||
| r'\/\/ Copyright( \([cC]\))? (\w+) .+\. {1,2}All rights reserved\.[\r\n]{1,2}' + |
There was a problem hiding this comment.
regexp is hard, but would this work with files containing multiple copyright lines?
wrt the comment on #851, I'm not even sure this is worth investigating.
| #!/bin/bash | ||
|
|
||
| if [ "$TRAVIS_PULL_REQUEST" != "false" ]; then | ||
| paths=$(git diff --diff-filter ACMR --name-only $TRAVIS_BRANCH -- test/) |
There was a problem hiding this comment.
--diff-filter d (lowercase d) is also a good option to avoid memorizing ACMR.
There was a problem hiding this comment.
That sounds like a good shortcut for typing, but for a script, I would prefer to be explicit.
|
@leobalter I just noticed that this patch was not passing its own tests. I've pushed up a commit that addresses the problem (it gets the tests to pass and includes the tests in the CI process). |
|
I'm merging this and I'll follow up at #851 with the copyright parts. |
| @@ -0,0 +1,2 @@ | |||
| # This file documents authorship information and is not itself a test | |||
This script is intended to identify common test file formatting errors
prior to their acceptance into the project. It is designed to support
future extensions for additional validation rules.
A good chunk of the new code here is for tests. I tried to make the format of
those tests as declarative as possible so that this would be easy to review and
maintain.