Add tests in tree to make sure we don't regress with clang-tidy

RESOLVED FIXED in Firefox 62

Status

enhancement
RESOLVED FIXED
Last year
Last year

People

(Reporter: sylvestre, Assigned: andi)

Tracking

(Blocks 2 bugs)

Trunk
mozilla62
Dependency tree / graph

Firefox Tracking Flags

(firefox60 wontfix, firefox61 wontfix, firefox62 fixed)

Details

Attachments

(2 attachments, 1 obsolete attachment)

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.

Updated

Last year
Product: Core → Firefox Build System
Reporter

Updated

Last year
Blocks: 1448008
Reporter

Updated

Last year
Duplicate of this bug: 1449638
Attachment #8965610 - Attachment is obsolete: true

Comment 4

Last year
mozreview-review
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]
Reporter

Updated

Last year
Assignee: nobody → bpostelnicu
Comment hidden (mozreview-request)
Attachment #8965611 - Attachment is obsolete: true
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Reporter

Updated

Last year
Depends on: 1452098
Reporter

Comment 8

Last year
mozreview-review
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.
Comment hidden (mozreview-request)
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.
Flags: needinfo?(ahalberstadt)
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 :).
Flags: needinfo?(ahalberstadt)
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
Comment hidden (mozreview-request)
Depends on: 1453709
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Depends on: 1380893
Windows target fails because of lacking public/build, this will be fixed by: 1380893
Comment hidden (mozreview-request)

Comment 21

Last year
mozreview-review
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?
Attachment #8965610 - Flags: review?(gps) → review-
Attachment #8965610 - Flags: review?(dustin)
Attachment #8965610 - Flags: review?(ahalberstadt)
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.
Comment hidden (mozreview-request)
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
Blocks: 1459862

Comment 26

Last year
mozreview-review
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.
Attachment #8965610 - Flags: review?(gps) → review+

Comment 27

Last year
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

Comment 28

Last year
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

Comment 29

Last year
bugherder
https://hg.mozilla.org/mozilla-central/rev/e6860d1adcc0
Status: NEW → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
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!
Flags: needinfo?(bpostelnicu)
(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!
Flags: needinfo?(bpostelnicu)
You need to log in before you can comment on or make changes to this bug.