1486866 - [mozlint] Fix errors around 'exclude' paths not being recognized

Status: RESOLVED FIXED

(Developer Infrastructure :: Lint and Formatting, defect)

Description

[Andrew Halberstadt [:ahal]]

While working on bug 1486863 I noticed a case where an 'exclude' path wasn't being picked up. While writing a test for that fix, I noticed another problem related to 'exclude' paths.

I figured I'd fix everything I encountered down this rabbit hole in the same bug. I'll of course add tests for all these cases along the way.

On one hand, I'm sad that I'm still finding these sorts of problems (see bug 1448417). But on the other hand, I was able to refactor the tests a bit and I now finally have an easy way to debug and add tests for these sorts of problems. So hopefully moving forward these will become less and less frequent as the test suite expands.

Comment 1

[Andrew Halberstadt [:ahal]]

Attachment: Bug 1486866 - [mozlint] Make sure exclude paths are relative to the 'path' being considered by the FileFinder, r=egao

Turns out FileFinder's 'ignore' variable expects all paths to be relative to
the search path. We weren't formatting these properly which was resulting in
the 'exclude' directives being ignored whenever linting something other than
root.

Depends on D5863

Comment 2

[Andrew Halberstadt [:ahal]]

Attachment: Bug 1486866 - [mozlint] Pass along exclude paths in the event an 'include' contains the path, r=egao

Depends on D5864

Comment 3

[Andrew Halberstadt [:ahal]]

Attachment: Bug 1486866 - [mozlint] Refactor test_include_exclude into parametrized test cases, r=egao

This will make it easier to add new test cases in the future. It'll
also make it easier to print debug, as only the output for each
specific test case will be printed on failure.

Comment 4

[Andrew Halberstadt [:ahal]]

Attachment: Bug 1486866 - [mozlint] Return extra 'excludes' directly from pathutils.filterpaths, r=egao

Currently pathutils.filterpaths computes some extra paths that the underlying linter
should try to exclude if it can. It sets these on lintargs['exclude'], and relies on
the fact that it was passed by reference to work.

However, it seems like pytest does some magic under the hood that prevents this value
from being propagated back. It will also fail if 'filterpaths' was invoked in a
subprocess.

It's much simpler and cleaner to pass this value back directly, rather than rely on a
reference.

Depends on D5862

Comment 5

[Edwin Takahashi (:egao | infrequent contributor)]

Comment on attachment 9009122 [details]
Bug 1486866 - [mozlint] Refactor test_include_exclude into parametrized test cases, r=egao

Edwin Gao ( :egao ) has approved the revision.

Comment 6

[Andrew Halberstadt [:ahal]]

While these changes are an improvement, I found a couple of issues at the last moment. I need to rethink this series and try to put changes into the right order for review. The first commit in the series is good to land though as soon as it gets r+ed.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0b14528bab1a
[mozlint] Refactor test_include_exclude into parametrized test cases, r=egao

Comment 8

[Andrew Halberstadt [:ahal]]

I got myself into a bit of a catch-22 with this bug and bug 1486863.

Basically this happened:

1. I started refactoring mozlint in bug 1486863
2. During testing, I discovered some path filtering issues and decided to fix them ahead of time (in this bug)
3. While working on fixing those issues, I discovered yet more issues and the scope here kept expanding
4. I realized the path filtering logic was *way* too complicated and refactored it such that it'll expand directories into individual files (which allowed me to get make fewer assumptions and get rid of tons of complexity).
5. The downside to that is that linting topsrcdir results in 1000's of paths being passed in, which has command lines that are too long for the OS to handle.
6. Luckily there is logic to chunk jobs into 50 paths at a time, but it gets applied too late.
7. The refactor in bug 1486863 would fix this automatically.

So now I'm in a situation where this patch is blocked on bug 1486863 and bug 1486863 is blocked on this patch, catch-22 :). Locally I've merged both the unsubmitted changes here and the refactor in the other bug together into a giant patch. Once I have everything working nicely, I'll try to split it up into other smaller pieces again. It was just too complicated for me to try and keep everything separate and green at the same time.

The result of all this is that I'm going to let this bug get closed, and just push ahead with the refactor. The issues that submitted patches for here, will be fixed along with the refactor (or will cease to exist because of the refactor).

Sorry for the bug churn.

Comment 9

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/0b14528bab1a