Add common sanity ignore & skip to ansible-test. #59416
Conversation
ansibot
added
affects_2.9
mattclay
removed
the
needs_triage
|
|
||
| if not ignore_entry: | ||
| continue | ||
| settings = self.load_settings(args, 'AnsibleTest') |
There was a problem hiding this comment.
I guess I don't fully understand what the code arg is for, why is this in PascalCase where most of the others is in snake_code?
There was a problem hiding this comment.
When ansible-test runs sanity tests, it checks the skip and ignore files for issues. If there are any to report, and the sanity test uses error codes, then ansible-test needs its own error code to report those errors. I chose to make the error codes ansible-test uses match the style of error code used by the sanity test for which the errors will be reported.
There was a problem hiding this comment.
This is briefly documented here: https://github.com/ansible/ansible/pull/59416/files#diff-74e3bc5c7fd66da8e877cdb786849182R462
There was a problem hiding this comment.
Just sounds somewhat arbitrary, from what I can see it's only in the actual error message. Just thinking that it should be the same for all or based on the test name to save passing another arg that is just used for reporting. I see it's just placed before the actual sanity issue like;
# from https://app.shippable.com/github/ansible/ansible/runs/133262/1/tests
test/sanity/pslint/ignore.txt:104:1: AnsibleTest Duplicate code 'PSAvoidUsingEmptyCatchBlock' for path 'lib/ansible/modules/windows/win_find.ps1' first found on line 101
I can see in this case it matches the PSAvoidUsingEmptyCatchBlock but considering it's placed before the actual sanity issue (Duplicate code...) it sounds like it should be based on the sanity test name. Even so when collections merge this into the 1 file wouldn't any test that accesses the file also produce a similar error that may or may not be related to it?
There was a problem hiding this comment.
Yes, the codes themselves are arbitrary. Although they're only for display purposes, I expect them to be included in documentation for the sanity tests at some point.
The argument cannot be removed since we need to know if the test uses codes or not when parsing the ignores. It could be made into a bool instead, if we didn't want to customize the code for each test.
I chose to match the style of codes used in each test so that results were more visually consistent within a given test. Here are some examples:
pylint
ERROR: lib/ansible/modules/system/ping.py:14:0: bare-except No exception type(s) specified
ERROR: test/sanity/pylint/ignore.txt:2:1: ansible-test Ignoring 'blacklisted-name' on 'lib/ansible/modules/system/ping.py' is unnecessary
pep8
ERROR: lib/ansible/modules/system/ping.py:14:1: E722 do not use bare 'except'
ERROR: test/sanity/pep8/skip.txt:4:1: A100 File 'does/not/exist.py' does not exist
If we can come up with code(s) that are more clear than the current implementation in the PR, I'm fine switching to something else. I just figured that having test results with mixed codes like "E772" and "ansible-test" would look a bit strange, particularly if they end up in tables or other columnar output.
As for the error handling when everything is combined into a single file, there will be a new sanity test that reports issues with the file itself, so some of the messages will move to that test. That will avoid issues with unrelated errors. However, there will always be sanity test specific messages from ansible-test that will appear within individual test results.
SUMMARY
Add common sanity ignore & skip to ansible-test.
This is the first step in moving towards a single unified ignore/skip file for all sanity tests. It consolidates the logic for processing ignore and skip files for all regular sanity tests, eliminating the code formerly implemented in each individual test.
Follow-up PRs will combine the existing ignore and skip files into a single file and then extend this common skip/ignore support for the existing code-smell tests.
ISSUE TYPE
Feature Pull Request
COMPONENT NAME
ansible-test