[mozlint] Error if unrecognized path is passed in

RESOLVED FIXED in Firefox 57

Status

enhancement
RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: ahal, Assigned: dyex719)

Tracking

unspecified
mozilla57

Firefox Tracking Flags

(firefox57 fixed)

Details

Attachments

(1 attachment, 4 obsolete attachments)

Reporter

Description

2 years ago
If a user passes an invalid path to |mach lint|, the command will happily ignore it and report 0 errors found. This could mislead the user to thinking that everything is ok when in reality there might be lint errors that went unreported.
Assignee

Comment 1

2 years ago
(In reply to Andrew Halberstadt [:ahal] from comment #0)
> If a user passes an invalid path to |mach lint|, the command will happily
> ignore it and report 0 errors found. This could mislead the user to thinking
> that everything is ok when in reality there might be lint errors that went
> unreported.

@Andrew
Hello! I am a newcomer! I hope to contribute but I need some help getting things started. You recommended this as a good starting point. Right now, I don't really understand what the bug is.
Flags: needinfo?(ahalberstadt)
Assignee

Comment 2

2 years ago
(In reply to dyex719 from comment #1)
> (In reply to Andrew Halberstadt [:ahal] from comment #0)
> > If a user passes an invalid path to |mach lint|, the command will happily
> > ignore it and report 0 errors found. This could mislead the user to thinking
> > that everything is ok when in reality there might be lint errors that went
> > unreported.
> 
> @Andrew
> Hello! I am a newcomer! I hope to contribute but I need some help getting
> things started. You recommended this as a good starting point. Right now, I
> don't really understand what the bug is.

I did bit of reading, and I understand that linters are used to check for correct coding practices and styles. From what I understand, the path specified for the user is not checked correctly. I am guessing we need to add a couple of lines like
/*
import os.path
os.path.exists(file_path)
*/
to check if the file or directory exists.
Suggestions would be appreciated.
Thank you!
Reporter

Comment 3

2 years ago
Hi dyex, yes you are correct! And sorry, I didn't get around to including much context :)

Mozlint is a library that takes a bunch of other linters (either third_party or made by mozilla) and runs them all at the same time. Here are some links:

Source code: https://dxr.mozilla.org/mozilla-central/source/python/mozlint
Linters: https://dxr.mozilla.org/mozilla-central/source/tools/lint
Docs: http://gecko.readthedocs.io/en/latest/tools/lint/index.html

For this bug you won't need to worry about the linters, you'll just be modifying the mozlint library itself. Hint: you probably only need to change the cli.py file. It would also be great to add a test for your change.

Thanks again for your interest! Let me know if you have any questions either around fixing the bug, or submitting patches for review.
Flags: needinfo?(ahalberstadt)
Assignee

Comment 4

2 years ago
(In reply to Andrew Halberstadt [:ahal] from comment #3)
> Hi dyex, yes you are correct! And sorry, I didn't get around to including
> much context :)
> 
> Mozlint is a library that takes a bunch of other linters (either third_party
> or made by mozilla) and runs them all at the same time. Here are some links:
> 
> Source code: https://dxr.mozilla.org/mozilla-central/source/python/mozlint
> Linters: https://dxr.mozilla.org/mozilla-central/source/tools/lint
> Docs: http://gecko.readthedocs.io/en/latest/tools/lint/index.html
> 
> For this bug you won't need to worry about the linters, you'll just be
> modifying the mozlint library itself. Hint: you probably only need to change
> the cli.py file. It would also be great to add a test for your change.
> 
> Thanks again for your interest! Let me know if you have any questions either
> around fixing the bug, or submitting patches for review.

Can you please assign me to this bug? I'll work on it asap.
I'll follow up with more questions soon.
Thank you :)
Assignee

Comment 5

2 years ago
I went to the repositry for the code that I needed access to and change, but when I fork the repository the entrire mozilla/positron repo gets forked which is quite big (~1 GB).

I was following this guide.
https://help.github.com/articles/fork-a-repo/

I never used git much before, but as far as I know, I have to identify where I need to add the required lines and send a pull request to you.

Is this correct? Do I have to clone both repositories (the cloned one and the original one) and then sync them?
After this, am I supposed to make changes to the code a send a Pull request?

I was thinking that there must be a way to fork  mozilla/positron/python/mozlint i.e only the files I require instead of the entire mozilla/positron.
Assignee

Comment 6

2 years ago
I also do not know who I test the changes I've made. I have built firefox from source but don't know how to proceed from here. Relatd web links would help me a lot.
Thank you
Assignee

Comment 7

2 years ago
ahal: Please take a look at this https://github.com/mozilla/positron/pull/155
How do I write a test for this? I am trying to use pytest. Do I have to use the function find_linters defined in cli.py and specifiy and example path?
Flags: needinfo?(ahalberstadt)
Reporter

Comment 8

2 years ago
Hi dyex, in order to write a test you can re-use the 'test_roller.py' test file. Simply add a new function there and copy how the other functions do it. You shouldn't need to make any new files for that. You can run the tests with:
./mach python-test python/mozlint

But your patch isn't checking the right thing, we want to validate the "paths" argument in the "run" function. Also you've found the wrong repository :). The source code lives in a mercurial repo:
https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Source_Code

There is also a read-only git clone if you prefer to use git, but we don't use pull requests and you won't be able to push anything to there:
https://github.com/mozilla/gecko-dev

Normally changes get pushed to a tool called mozreview, and review is conducted there, but for now you can simply upload a patch as an attachment to this bug. I'd also recommend taking a look through our contribution documentation:
https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Introduction
https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide
Assignee: nobody → dyex719
Status: NEW → ASSIGNED
Flags: needinfo?(ahalberstadt)
Assignee

Comment 9

2 years ago
I think I understand what I did wrong.

Is this how I add a patch? Is there a particular format for doing so?
I will add the code for testing, if this is correct.
Attachment #8889470 - Flags: feedback+
Reporter

Comment 10

2 years ago
Not quite, you need to provide a diff, and while uploading check the 'patch' box. The diff needs to include the commit metadata, like author name and patch title. You can use `hg export` to do this in mercurial, see:
https://developer.mozilla.org/en-US/docs/Mercurial/Using_Mercurial#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F

I think on git, you could use git show. But please make sure you are rebased on the latest source, that positron repo is likely out of date.
Indeed, the Positron repo is very out-of-date, so it isn't a good repo to use for this change.  You should use mozilla/gecko-dev <https://github.com/mozilla/gecko-dev>.

Once you've cloned that repo, created a branch, and committed your changes to it, generate a patch using `git format-patch -k -p master`.

Also note these recommendations for how to write the commit message <https://developer.mozilla.org/en-US/docs/Mercurial/Using_Mercurial#Commit_Message_Conventions>.
Assignee

Comment 12

2 years ago
Please review the above. Sorry for the delay.
Attachment #8892723 - Flags: review+
Attachment #8892723 - Flags: feedback+
Reporter

Comment 13

2 years ago
Comment on attachment 8892723 [details] [diff] [review]
Using os.path.exists to check if the path that is entered by user is a valid one.

Thanks dyex! No worries about the delay.

For the future, to request a review you flip the "review" flag to ? and then put my name in the "Requestee" box (e.g :ahal). Then when I've finished reviewing, I'll either set it to + or -.
Attachment #8892723 - Flags: review?(ahalberstadt)
Attachment #8892723 - Flags: review+
Attachment #8892723 - Flags: feedback+
Reporter

Comment 14

2 years ago
Comment on attachment 8892723 [details] [diff] [review]
Using os.path.exists to check if the path that is entered by user is a valid one.

Review of attachment 8892723 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for the patch! I think putting a 'print' statement in a list comprehension is a syntax error in Python 2 (it will work in python 3 though, but still likely not do what you expect).

You can run the mozlint tests by doing:
./mach python-test python/mozlint

That should give you an idea if the patch is working or not. I also recommend you use `hg export` to generate your patches, e.g:
$ hg export > bug1369710.patch

Then upload the resulting file.

Thanks for sticking with this, you're getting close!

::: patch.py
@@ +222,5 @@
>      lint = LintRoller(**lintargs)
>      lint.read(find_linters(linters))
>  
> +    # Check if the path that is entered is a valid one.
> +    paths = [path if os.path.exists(path) else print('specified path ' + path + ' does not exist') for path in paths]

I'd lets use a list comprehension to get all the missing paths, and then do something like:
print("The following paths do not exist:\n{}".format("\n".join(missing))

afterwards.
Attachment #8892723 - Flags: review?(ahalberstadt) → review-
Reporter

Updated

2 years ago
Attachment #8889470 - Attachment is obsolete: true
Assignee

Comment 15

2 years ago
Posted patch bug1369710.patch (obsolete) — Splinter Review
How do I write a function that checks if the correct path is entered in?

I tried something like:
@pytest.fixture(params=[
    './linters',                  #this path exists
    './linters/structured.yml',   #this path exists
    './files/foobar.js',          #this path exists
    './files/no_such_dir'         #this path doesn't exist
    './files/no_such_file.py'])   #this path doesn't exist
def test_valid_path(request):
    assert(os.path.exists(request.param) == True) # this will give an error

For some reason the test does not run, and I get 'no tests ran in 0.00s'
Attachment #8893659 - Flags: review?(ahalberstadt)
Reporter

Comment 16

2 years ago
I think you want something like:

@pytest.parametrize('path', [
    'path/foo',
    'path/bar'])
def test_valid_path(path):
    ...

Thanks for thinking about a test! But this check isn't to tell whether or not the files checked under 'linters' and 'files' ree exist or not. It's to check whether what the user passes in exists on the command line. For example, when they run:
./mach lint path/to/foo

We're making sure that 'path/to/foo' exists. If you want it's probably fine to skip a test for this, though if you'd like to try and figure out adding a test, please feel free!
Reporter

Comment 17

2 years ago
Comment on attachment 8893659 [details] [diff] [review]
bug1369710.patch

Review of attachment 8893659 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks again, almost there!

::: python/mozlint/mozlint/cli.py
@@ +114,5 @@
>      lint.read(find_linters(linters))
>  
> +    # Check if the path that is entered is a valid one.
> +    missing_paths = [path if os.path.exists(path) for path in paths]
> +    print("The following paths do not exist:\n{}".format("\n".join(missing_paths))

This will print the message every time, even if there's no missing tests. You'll want to put this print behind an if statement.

Also, I think we should error out if there are missing paths, so please add a 'return 1' after the print. To be more clear that it's an error, please change the message to "error: following paths do not..."
Attachment #8893659 - Flags: review?(ahalberstadt) → review-
Reporter

Updated

2 years ago
Attachment #8892723 - Attachment is obsolete: true
Assignee

Comment 18

2 years ago
Posted patch bug1369710.patch (obsolete) — Splinter Review
Am I missing anything?
Attachment #8894211 - Flags: review?(ahalberstadt)
Attachment #8894211 - Flags: feedback+
Reporter

Comment 19

2 years ago
Comment on attachment 8894211 [details] [diff] [review]
bug1369710.patch

Review of attachment 8894211 [details] [diff] [review]:
-----------------------------------------------------------------

Awesome, the code is good! Just one more thing though, please change the commit message to reflect Mozilla's conventions, as per:
https://developer.mozilla.org/en-US/docs/Mercurial/Using_Mercurial#Commit_Message_Conventions

It should have the bug number, message (please mention it's for mozlint) and r=ahal at the end. Thanks again for your contribution!
Attachment #8894211 - Flags: review?(ahalberstadt) → review+
Reporter

Updated

2 years ago
Attachment #8893659 - Attachment is obsolete: true
Assignee

Comment 20

2 years ago
Thank you for all your help!
Attachment #8894874 - Flags: review?(ahalberstadt)
Reporter

Comment 21

2 years ago
Comment on attachment 8894874 [details] [diff] [review]
bug1369710.patch

Review of attachment 8894874 [details] [diff] [review]:
-----------------------------------------------------------------

I'll get this landed on your behalf later today, you'll see a message posted here by a bot when it gets merged. Thanks for your contribution!
Attachment #8894874 - Flags: review?(ahalberstadt) → review+
Reporter

Updated

2 years ago
Attachment #8894211 - Attachment is obsolete: true

Comment 22

2 years ago
Pushed by ahalberstadt@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9f2c22f4ef01
[mozlint] Ensure that a valid path is entered. r=ahal

Comment 23

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/9f2c22f4ef01
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57

Updated

a year ago
Product: Testing → Firefox Build System
You need to log in before you can comment on or make changes to this bug.