Closed Bug 1472975 Opened 6 years ago Closed 6 years ago

Add tests in tree to make mach static-analysis doesn't regress.

Categories

(Developer Infrastructure :: Source Code Analysis, enhancement)

enhancement
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: andi, Assigned: andi)

References

Details

Attachments

(1 obsolete file)

The need of this bug was released after this Bug 1472721. We need tests to verify our entire mach static-analysis pipeline, the current tests that we have do:
1. verify that artifacts exist for clang-tidy [a]
2. verify that the current artifacts have the same checkers and yield the same results as we expect, based on a test suit. [b]

What we don't have is:
1. Verify that the current mach static-analysis pipeline performs as expected with different number of files.[c] 

Due to [c] we know that it performs well with a single file.
Assignee: nobody → bpostelnicu
What this patch does is to wrap the run-clang-tidy in a single function that is callled when we want to run it in two places:
1. regular static-analysis
2. testing 

By doing so we are sure that modifying the command will mitigate the risk of adding regressions since the tests will use the same command.
Comment on attachment 8995085 [details]
WIP WIP - Bug 1472975 - Part 2 - add tests for running several files.

https://reviewboard.mozilla.org/r/259576/#review266590

Thank you for increasing the coverage of our tests! R+ with a few nits (please see below).

Also, since today we only autotest our analysis on a single file at a time, I don't think we would actually catch regressions like the header_filter problem we had before. Please open a follow-up bug to also test with multiple files.

::: python/mozbuild/mozbuild/mach_commands.py:1715
(Diff revision 1)
>              self.log(logging.WARNING, 'warning_summary',
>                       {'count': len(monitor.warnings_db)},
>                       '{count} warnings present.')
>              return rc
>  
> +    def _get_clang_tidy_command(self, checks, header_filter, source, jobs, fix):

Nit: Maybe `s/source/sources/` could make it more obvious that it's a list and not a single source path?

::: python/mozbuild/mozbuild/mach_commands.py:1723
(Diff revision 1)
> +            checks = self._get_checks()
> +
> +        common_args = [
> +            '-checks=%s' % checks, '-extra-arg=-DMOZ_CLANG_PLUGIN', '-clang-tidy-binary',
> +            self._clang_tidy_path, '-clang-apply-replacements-binary',
> +            self._clang_apply_replacements

Nit: Why shuffle these arguments around? I found the original layout more readable (see left view of patch).

::: python/mozbuild/mozbuild/mach_commands.py:1953
(Diff revision 1)
>          # Verify if the test file exists for this checker
>          if not os.path.exists(test_file_path_cpp):
>              self.log(logging.ERROR, 'static-analysis', {}, "ERROR: clang-tidy checker {} doesn't have a test file.".format(check))
>              return self.TOOLS_CHECKER_NO_TEST_FILE
>  
> -        cmd = [self._clang_tidy_path, '-checks=-*, ' + check, test_file_path_cpp]
> +        cmd = self._get_clang_tidy_command('-*,' + check, '', [test_file_path_cpp], 1, False)

Nit: Maybe it could be helpful to name the function arguments, e.g. like so?

```python
_get_clang_tidy_command(checks='-*,' + check, header_filter='', source=[test_file_path_cpp], jobs=1, fix=False)
```
Attachment #8995085 - Flags: review?(janx) → review+
I will followup with another patch that handles the multi-files analysis, that why I've marked it as leave open.
Keywords: leave-open
Pushed by bpostelnicu@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/4a8c07c64ed9
Add tests in tree to make mach static-analysis doesn't regress. r=janx
Depends on: 1478644
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
No longer depends on: 1478644
Comment on attachment 8995085 [details]
WIP WIP - Bug 1472975 - Part 2 - add tests for running several files.

I don't remember ever r+'ing this.

Please don't re-use merged MozReview (or Phabricator) reviews for follow-up patches, but create separate reviews for them.
Attachment #8995085 - Flags: review+
(In reply to Jan Keromnes [:janx] from comment #11)
> Comment on attachment 8995085 [details]
> WIP WIP - Bug 1472975 - Part 2 - add tests for running several files.
> 
> I don't remember ever r+'ing this.
> 
> Please don't re-use merged MozReview (or Phabricator) reviews for follow-up
> patches, but create separate reviews for them.

that patch didn't land because of the r+, I've opened another bug for that patch: https://reviewboard.mozilla.org/r/260776/diff/1#index_header
Additionally, if you'd like to continue working on this patch, you'll probably want to re-open this bug and also re-add the removed "leave-open" keyword.
(In reply to Jan Keromnes [:janx] from comment #13)
> Additionally, if you'd like to continue working on this patch, you'll
> probably want to re-open this bug and also re-add the removed "leave-open"
> keyword.

No need, I've moved all of the work here: https://bugzilla.mozilla.org/show_bug.cgi?id=1480089
In this way it's clearer what our intentions are.
Ok, work should continue in bug 1480089 then. I'll mark this attachment as obsolete, because the patch on MozReview is different from what has landed in the tree.
Attachment #8995085 - Attachment is obsolete: true
Blocks: 1480089
Product: Firefox Build System → Developer Infrastructure
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: