We should add some tests here: https://dxr.mozilla.org/mozilla-central/source/build/clang-plugin/tests for clang-tidy regular checkers listed here: https://dxr.mozilla.org/mozilla-central/source/tools/clang-tidy/config.yaml we should have a test per checker.

Comment on attachment 8965610 [details] Bug 1432410 - Add tests in tree to make sure we don't regress with clang-tidy on static-analisys. Tests wrote in part by :sylvestre. https://reviewboard.mozilla.org/r/234456/#review240030 Code analysis found 10 defects in this patch: - 10 defects found by mozlint You can run this analysis locally with: - `./mach lint path/to/file` (JS/Python) If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx ::: tools/clang-tidy/test/conftest.py:1 (Diff revision 1) > +# Any copyright is dedicated to the Public Domain. Error: Missing from __future__ import absolute_import [py2: require absolute_import] ::: tools/clang-tidy/test/conftest.py:1 (Diff revision 1) > +# Any copyright is dedicated to the Public Domain. Error: Missing from __future__ import absolute_import [py2: require absolute_import] ::: tools/clang-tidy/test/conftest.py:6 (Diff revision 1) > +# Any copyright is dedicated to the Public Domain. > +# http://creativecommons.org/publicdomain/zero/1.0/ > + > +import os > +import sys > +from collections import defaultdict Error: 'collections.defaultdict' imported but unused [flake8: F401] ::: tools/clang-tidy/test/conftest.py:9 (Diff revision 1) > +import os > +import sys > +from collections import defaultdict > + > +from mozbuild.base import MozbuildObject > +from mozlint.pathutils import findobject Error: 'mozlint.pathutils.findobject' imported but unused [flake8: F401] ::: tools/clang-tidy/test/conftest.py:10 (Diff revision 1) > +import sys > +from collections import defaultdict > + > +from mozbuild.base import MozbuildObject > +from mozlint.pathutils import findobject > +from mozlint.parser import Parser Error: 'mozlint.parser.parser' imported but unused [flake8: F401] ::: tools/clang-tidy/test/conftest.py:12 (Diff revision 1) > + > +from mozbuild.base import MozbuildObject > +from mozlint.pathutils import findobject > +from mozlint.parser import Parser > + > +import pytest Error: 'pytest' imported but unused [flake8: F401] ::: tools/clang-tidy/test/test_clang_tidy.py:1 (Diff revision 1) > +# Any copyright is dedicated to the Public Domain. Error: Missing from __future__ import absolute_import [py2: require absolute_import] ::: tools/clang-tidy/test/test_clang_tidy.py:1 (Diff revision 1) > +# Any copyright is dedicated to the Public Domain. Error: Missing from __future__ import absolute_import [py2: require absolute_import] ::: tools/clang-tidy/test/test_clang_tidy.py:4 (Diff revision 1) > +# Any copyright is dedicated to the Public Domain. > +# http://creativecommons.org/publicdomain/zero/1.0/ > + > +import mozunit Error: 'mozunit' imported but unused [flake8: F401] ::: tools/clang-tidy/test/test_clang_tidy.py:5 (Diff revision 1) > +# Any copyright is dedicated to the Public Domain. > +# http://creativecommons.org/publicdomain/zero/1.0/ > + > +import mozunit > +import pytest Error: 'pytest' imported but unused [flake8: F401]

Comment on attachment 8965610 [details] Bug 1432410 - Add tests in tree to make sure we don't regress with clang-tidy on static-analisys. Tests wrote in part by :sylvestre. https://reviewboard.mozilla.org/r/234456/#review240166 ::: python/mozbuild/mozbuild/mach_commands.py:1698 (Diff revision 4) > {'count': len(monitor.warnings_db)}, > '{count} warnings present.') > return rc > > + @StaticAnalysisSubCommand('static-analysis', 'autotest', > + 'Run the auto-test suit in order to determine that' s/suit/suite/ ::: python/mozbuild/mozbuild/mach_commands.py:1699 (Diff revision 4) > '{count} warnings present.') > return rc > > + @StaticAnalysisSubCommand('static-analysis', 'autotest', > + 'Run the auto-test suit in order to determine that' > + ' the analysis didn\'t regress.') did not ::: python/mozbuild/mozbuild/mach_commands.py:1710 (Diff revision 4) > + def autotest(self, verbose=False, dump_results=False, checker_names=[]): > + # If dump-results is True than we just wanna generate the issues.json, we will not > + # force the re-dowloand from artifacts > + self._set_log_level(verbose) > + > + force = False maybe rename the variable: force what? :) ::: python/mozbuild/mozbuild/mach_commands.py:1722 (Diff revision 4) > + if rc != 0: > + raise Exception('clang-tidy unable to download package from artifacts build!') > + return rc > + > + # 3. For each checker run it > + with open(mozpath.join(self.topsrcdir, "tools", "clang-tidy", "config.yaml")) as f: You have a lot of occurences to the path self.topsrcdir, "tools", "clang-tidy" maybe move that into a single declaration clang_tidy_base_path ? ::: python/mozbuild/mozbuild/mach_commands.py:1727 (Diff revision 4) > + with open(mozpath.join(self.topsrcdir, "tools", "clang-tidy", "config.yaml")) as f: > + import yaml > + config = yaml.load(f) > + for item in config['clang_checkers']: > + # Do not test mozilla specific checks nor the default '-*' > + if item['publish'] and item['name'] not in ['mozilla-*', '-*'] and (checker_names == [] or item['name'] in checker_names): the list is pretty long, maybe move that into a function? ::: python/mozbuild/mozbuild/mach_commands.py:1733 (Diff revision 4) > + check = item['name'] > + test_file_name = mozpath.join(self.topsrcdir, "tools", "clang-tidy", "test", check) > + > + cmd = [ > + self._clang_tidy_path, > + '-checks=-*, ' + check, maybe use .format here ::: python/mozbuild/mozbuild/mach_commands.py:1743 (Diff revision 4) > + > + issues = self._parse_issues(clang_output) > + > + # Verify to see if we got any issues, if not raise exception > + if len(issues) == 0: > + raise Exception('clang-tidy checker %s didn\'t find any issues in it\'s associated test suit.' % check) did ont + suite ::: python/mozbuild/mozbuild/mach_commands.py:1819 (Diff revision 4) > self._clang_format_path, show) > else: > return self._run_clang_format_path(self._clang_format_path, show, path) > > + def _build_autotest_result(self, file, issues): > + with open(mozpath.join(self.topsrcdir, "tools", "clang-tidy", "test", file+".json"), 'w') as f: almost duplicated code from 5 lines under ::: tools/clang-tidy/config.yaml:61 (Diff revision 4) > - publish: !!bool yes > + publish: !!bool no > - name: modernize-use-override > # Too noisy because of the way how we implement NS_IMETHOD. See Bug 1420366. > publish: !!bool no > - name: mozilla-* > + publish: !!bool no This should probably be done in a separate commit ::: tools/clang-tidy/test/performance-for-range-copy.cpp:1 (Diff revision 4) > +namespace std { we can probably remove that.

Andrew can you please provide feedback on how we can integrate the current patch with mozlint autotest? My final goal is to have something very similar with this: https://reviewboard.mozilla.org/r/230640/diff/3#index_header with a try task running.

It looks like this is creating a new test harness independent of the existing |mach python-test| test harness that mozlint is using? Is this a standard clang-tidy thing? Maybe that's a source of confusion. The patch in https://reviewboard.mozilla.org/r/230640/diff/3#index_header didn't create a new framework or anything, it's using the pre-existing |mach python-test| framework (that is used for tens of different libraries all around the tree). But generally, if your goal is to create some new python-tests like the ones I added in https://reviewboard.mozilla.org/r/230640/diff/3#index_header, you won't be creating any new mach commands. Here are the rough steps: 1) Create a python.ini file that lists your tests and adds a subsuite to the DEFAULT section 2) Add that manifest to the nearest moz.build in the PYTHON_UNITTEST_MANIFESTS variable 3) Create a new entry in taskcluster/ci/source-test/python.yml to get a task running (just copy/paste an existing task and modify) Then you'll actually write the tests which are using the 'pytest' framework: https://docs.pytest.org/en/latest/ How you do this would be up to you, but you could for example, invoke your static-analyzers in a subprocess and assert the output is what you expect. It's possible |mach python-test| isn't the right framework for this use case. In that case maybe you'd be better off creating a new framework, but from experience that will be *a lot* more work and be an ongoing maintenance burden for you :).

On the flip side, if you just want to move forward with the patch you have and get it running in CI, you'll have to create a new file in taskcluster/ci/source-test. There won't be much prior art to follow, but I'm happy to help if you run into any specific problems.

Thanks for the input, I've created a new mach command since we also need a CLI command in order to generate the new defects when migrating to a new clang-tidy version, also I wanted to give the possibility for the engineer to locally validate the outcome of clang-tidy on a default set of tests. I think on the way how the current mach command is structured I can add it to taskcluster as a separate command and lunch './mach static-analysis autotest' and in case of regression it will fail. Do you think this is feasible? Also if i'm correctly interpreting the rules from taskcluster/ci/source-test/python.yml I can set it up to trigger only when specific files from the tree get changed. I will update this patch shortly with the new task

Windows target fails because of lacking public/build, this will be fixed by: 1380893

Comment on attachment 8965610 [details] Bug 1432410 - Add tests in tree to make sure we don't regress with clang-tidy on static-analisys. Tests wrote in part by :sylvestre. https://reviewboard.mozilla.org/r/234456/#review247148 Most of the comments are nits or things that can be addressed with follow-ups. But there are some aspects with regards to the taskgraph configuration that need further consideration. I'll flag a few more people to look at this... ::: python/mozbuild/mozbuild/mach_commands.py:1757 (Diff revision 10) > + self._clang_tidy_base_path = mozpath.join(self.topsrcdir, "tools", "clang-tidy") > + > + # For each checker run it > + with open(mozpath.join(self._clang_tidy_base_path, "config.yaml")) as f: > + import yaml > + config = yaml.load(f) Nit: everything below could be dedented since we don't access the file after parsing it. ::: python/mozbuild/mozbuild/mach_commands.py:1767 (Diff revision 10) > + if item['publish'] and ( > + 'restricted-platforms' in item > + and platform not in item['restricted-platforms'] > + or 'restricted-platforms' not in item) and item['name'] not in [ > + 'mozilla-*', '-*' > + ] and (checker_names == [] or item['name'] in checker_names): Nit: if this condition were inverted, you could `continue` and dedent all lines below. (I don't like when Python code resembles the "pyramid of doom" made notorious by JavaScript because it makes control flow harder to read.) ::: python/mozbuild/mozbuild/mach_commands.py:1787 (Diff revision 10) > + clang_output = subprocess.check_output( > + cmd, stderr=subprocess.STDOUT).decode('utf-8') As a potential follow-up, would it be possible to invoke multiple analyzers concurrently? If Clang is only using a single thread for the analysis, we should highly consider using a `concurrent.futures.ThreadPoolExecutor` here to spawn multiple processes so the static analysis checks complete faster. Especially if we expect this command to be in our developer's standard workflows. This is definitely the territory of a follow-up though. ::: python/mozbuild/mozbuild/mach_commands.py:1793 (Diff revision 10) > + cmd, stderr=subprocess.STDOUT).decode('utf-8') > + > + issues = self._parse_issues(clang_output) > + > + # Verify to see if we got any issues, if not raise exception > + if issues == []: Nit: `if not issues` is more Pythonic. ::: python/mozbuild/mozbuild/mach_commands.py:1888 (Diff revision 10) > else: > return self._run_clang_format_path(self._clang_format_path, show, path) > > + def _build_autotest_result(self, file, issues): > + with open(file, 'w') as f: > + json.dump(issues, f) We like using `sort_keys=True, indent=4` when writing JSON so output is deterministic and easier for humans to read. This helps so much with debugging and rarely causes any performance issues. ::: taskcluster/ci/static-analysis-autotest/kind.yml:25 (Diff revision 10) > + linux64-st-autotest/debug: > + description: "Linux64 Debug Static Analysis Autotest" > + index: > + job-name: linux64-st-autotest-debug > + treeherder: > + platform: linux64/debug Should this be using `linting` as the "platform" value and should the task name, description, and treeherder platform reflect that this is closer to a source lint than a build task? (The whole meaning of "platform" in Treeherder is kind of bonkers. But we do have a "linting" platform for holding static analysis type things.) ::: taskcluster/ci/static-analysis-autotest/kind.yml:31 (Diff revision 10) > + description: "Linux64 Debug Static Analysis Autotest" > + index: > + job-name: linux64-st-autotest-debug > + treeherder: > + platform: linux64/debug > + worker-type: aws-provisioner-v1/gecko-{level}-b-linux This will build on workers with 16 virtual CPUs. Assuming Clang is single threaded and we're running a single concurrent process, this is quite wasteful of the hardware resources. If the mach command uses all available CPU cores and we can saturate >50% of the CPU, then the gecko-{level}-b-linux worker type is acceptable. If it can't, then we should move to a smaller worker. If there are no security concerns with operations of this task running on the same machines where lesser-privileged tasks run, then we should move it to something like the `gecko-t-linux-large` worker. Although that's an `m3.large`. We have a gap in our worker coverage and don't seem to have a [cm]5.large worker type. Bleh. Maybe Dustin or wcosta can hook us up with something to plug that gap. ::: taskcluster/ci/static-analysis-autotest/kind.yml:33 (Diff revision 10) > + job-name: linux64-st-autotest-debug > + treeherder: > + platform: linux64/debug > + worker-type: aws-provisioner-v1/gecko-{level}-b-linux > + worker: > + max-run-time: 36000 10 hours is a bit excessive. If there is a bug in the task where something busy loops or waits on something that's never going to return, this will result in a worker sitting around for 10 hours doing effectively nothing of value and wasting money. Could this be reduced to something more reasonable?

I want to address some concerns that you very well raised them. 1. Parallelisation a. The test suit is very small, 43 cpp files, with extremely little content inside them. b. The command is not intended to be ran by the engineer, since it doesn't show him relevant information, it will only show that the current static-analysis build is sane. This was developed to be use on automation to be sure that we don't regress when updating stuff from build/build-clang c. The actual overall speed on which the test is ran now, is around 2s for the entire suit of checkers. d. Of course I can do a followup and have it split in multi-process. 2. Task Cluster Task I've decided to use the build task having in mind several criteria like: a. The need to configure the tree, thus having access to the build tools b. The need to use the artifacts, like clang-tidy c. I want to have it multi-platform and in a future bug, after bug 1380893 lands. d. I've used as inspiration the static-analysis job so maybe as you said I can optimise something to my task.

(In reply to Andi-Bogdan Postelnicu [:andi] from comment #22) > b. The command is not intended to be ran by the engineer, since it > doesn't show him relevant information, it will only show that the current > static-analysis build is sane. This was developed to be use on automation to > be sure that we don't regress when updating stuff from build/build-clang When it goes wrong, someone will need to fix it, and that person will likely want to run it locally. So it's still important to be able to run it locally. Given the size and speed of the harness, I think it's fine to punt on parallelization. > 2. Task Cluster Task > I've decided to use the build task having in mind several criteria like: > a. The need to configure the tree, thus having access to the build tools > b. The need to use the artifacts, like clang-tidy > c. I want to have it multi-platform and in a future bug, after bug > 1380893 lands. > d. I've used as inspiration the static-analysis job so maybe as you said > I can optimise something to my task. Yeah, I agree. As this is platform dependent, I think it should show up on the platform it's running under. The "Linting opt" platform (which I'd love if we renamed to "Linux64 src"), is more for tasks that don't depend on any build configuration and (theoretically) are platform independent.

Also in order to be sure that this patch doesn't break anything on try, this is the latest push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f6719ac738017a6263607cb6185bb459f4e71d80&selectedJob=176796078

Comment on attachment 8965610 [details] Bug 1432410 - Add tests in tree to make sure we don't regress with clang-tidy on static-analisys. Tests wrote in part by :sylvestre. https://reviewboard.mozilla.org/r/234456/#review249666 This looks pretty reasonable. I'm not sure that we needed to introduce a new Taskcluster kind just to hold tests for this. But that's mostly a cosmetic thing and it shouldn't matter much.

Pushed by bpostelnicu@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/e6860d1adcc0 Add tests in tree to make sure we don't regress with clang-tidy on static-analisys. Tests wrote in part by :sylvestre. r=gps

Pushed by csabou@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/f1de7aa43228 Add tests in tree to make sure we don't regress with clang-tidy on static-analisys. Tests wrote in part by :sylvestre. r=gps

Andi, in your /tools/clang-tidy/config.yaml refactor from https://hg.mozilla.org/mozilla-central/rev/f1de7aa43228, you appear to have made a few drive-by changes: 1. Drive-by changed "clang-analyzer-security.insecureAPI.rand" from "publish: !!bool yes" to "publish: !!bool no". Why? 2. Restricted "modernize-avoid-bind" to "win32" and "win64" only (I guess because this rule is platform-specific) 3. Replacing the wildcard "performance-*" with all explicit performance checks (except "performance-faster-string-find", which I re-added in bug 1468811) Also, in the future, please refrain from making drive-by changes while refactoring, or at least document all these changes (e.g. in the patch summary, or better as a comment in the code directly) because otherwise it gets very hard for us to understand why a configuration was changed (i.e. is this checker disabled for a valid reason, or is it just a mistake / a bug?) Thanks!

(In reply to Jan Keromnes [:janx] from comment #31) > Andi, in your /tools/clang-tidy/config.yaml refactor from > https://hg.mozilla.org/mozilla-central/rev/f1de7aa43228, you appear to have > made a few drive-by changes: > > 1. Drive-by changed "clang-analyzer-security.insecureAPI.rand" from > "publish: !!bool yes" to "publish: !!bool no". Why? This has been deprecated for a long time. Also the underlying reason on why not to have it, it's very debatable. > > 2. Restricted "modernize-avoid-bind" to "win32" and "win64" only (I guess > because this rule is platform-specific) Yes, this is something that is strictly related with win* platforms, at least in our case. Also most of the usage is in test files. The end goal of this checker is the easy of read but also this is highly debatable. There is the factor of cpp dialect. There are still some binds because until cpp11 bind could be use in more cases than the lambda permitted. > > 3. Replacing the wildcard "performance-*" with all explicit performance > checks (except "performance-faster-string-find", which I re-added in bug > 1468811) This was done because of several reasons: 1. A better control on what actual checkers from the performance namespace we want to have 2. We introduced per checker tests, and it was way more easier to explicitly specify each checker. 3. Avoid regression upon on a clang-tidy upgrade, what if we upgrade clang-tidy and we get some new checkers from performance namespace that don't act well in our code base? We could be flooded with false positives. I would expect that when doing a clang-tidy update we would monitor the checkers for name changes and updates, in order to modify this list accordingly. > > Also, in the future, please refrain from making drive-by changes while > refactoring, or at least document all these changes (e.g. in the patch > summary, or better as a comment in the code directly) because otherwise it > gets very hard for us to understand why a configuration was changed (i.e. is > this checker disabled for a valid reason, or is it just a mistake / a bug?) Completely agree with you, but when we did this kind of modifications, we had a discussions on what we should enable/disable, at least Sylvestre and I had. > > Thanks!