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)
Developer Infrastructure
Source Code Analysis
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 | ||
Updated•6 years ago
|
Assignee: nobody → bpostelnicu
Comment hidden (mozreview-request) |
Assignee | ||
Comment 2•6 years ago
|
||
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.
Assignee | ||
Comment 3•6 years ago
|
||
Try test for win64 https://treeherder.mozilla.org/#/jobs?repo=try&revision=64e7f928447212afcc1d41deaadf2cf190ee59f5
Assignee | ||
Comment 4•6 years ago
|
||
Try test for linux64 https://treeherder.mozilla.org/#/jobs?repo=try&revision=d2ca0168ec31d44d17db208a93e4ac30ce0ce0f6
Comment 5•6 years ago
|
||
mozreview-review |
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+
Comment hidden (mozreview-request) |
Assignee | ||
Comment 7•6 years ago
|
||
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
Comment 9•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/4a8c07c64ed9
Comment hidden (mozreview-request) |
Assignee | ||
Updated•6 years ago
|
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Updated•6 years ago
|
Keywords: leave-open
Comment 11•6 years ago
|
||
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+
Assignee | ||
Comment 12•6 years ago
|
||
(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
Comment 13•6 years ago
|
||
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.
Assignee | ||
Comment 14•6 years ago
|
||
(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.
Comment 15•6 years ago
|
||
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.
Updated•6 years ago
|
Attachment #8995085 -
Attachment is obsolete: true
Updated•2 years ago
|
Product: Firefox Build System → Developer Infrastructure
You need to log in
before you can comment on or make changes to this bug.
Description
•