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.

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.

Try test for win64 https://treeherder.mozilla.org/#/jobs?repo=try&revision=64e7f928447212afcc1d41deaadf2cf190ee59f5

Try test for linux64 https://treeherder.mozilla.org/#/jobs?repo=try&revision=d2ca0168ec31d44d17db208a93e4ac30ce0ce0f6

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) ```

I will followup with another patch that handles the multi-files analysis, that why I've marked it as 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

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.

(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.