Closed Bug 1431024 Opened 6 years ago Closed 6 years ago

[mozprofile] File prefs.py never raises error PreferencesReadError for invalid types

Categories

(Testing :: Mozbase, defect)

Version 2
defect
Not set
normal

Tracking

(firefox60 fixed)

RESOLVED FIXED
mozilla60
Tracking Status
firefox60 --- fixed

People

(Reporter: vedantc98, Assigned: vedantc98, Mentored)

Details

Attachments

(1 file)

In prefs.py, `[isinstance(i, j) for j in types]` returns a 3-element array, which will always evaluate to true.
Thus, the types of values are never tested and this never returns an error.

Here is the offending line:
https://dxr.mozilla.org/mozilla-central/source/testing/mozbase/mozprofile/mozprofile/prefs.py#151
Assignee: nobody → vedantc98
William, should that be enough to fix this?
Thanks.
Comment on attachment 8943177 [details]
Bug 1431024 - Modified error criterion check in prefs.py.

https://reviewboard.mozilla.org/r/213516/#review219300

Looks ok, would you care to write a test for this case?

I would probably put it in here:

testing/mozbase/mozprofile/tests/test_preferences.py
Attachment #8943177 - Flags: review?(wlachance) → review-
I've pushed another patch, although I think there might be a few issues.
For one, I haven't tested the precise error message, only whether or not the error is raised.
The problem is unittest assertRaises, which gives an attributeError(__exit__) when it is used without a context manager.
Comment on attachment 8943177 [details]
Bug 1431024 - Modified error criterion check in prefs.py.

https://reviewboard.mozilla.org/r/213516/#review220910


Static analysis found 3 defects in this patch.
 - 3 defects found by mozlint

You can run this analysis locally with:
 - `./mach lint check path/to/file` (Python/Javascript/wpt)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: testing/mozbase/mozprofile/mozprofile/prefs.py:151
(Diff revision 2)
>          elif isinstance(prefs, dict):
>              values = prefs.values()
>          else:
>              raise PreferencesReadError("Malformed preferences: %s" % path)
>          types = (bool, string_types, int)
> -        if [i for i in values if not [isinstance(i, j) for j in types]]:
> +        if [i for i in values if not True in [isinstance(i, j) for j in types]]:

Error: test for membership should be 'not in' [flake8: E713]

::: testing/mozbase/mozprofile/tests/test_preferences.py:310
(Diff revision 2)
>              commandline = ["--preferences", f.name]
>              self.compare_generated(_prefs, commandline)
>  
> +    def test_json_datatypes(self):
> +        json = """{"zoom.minPercent": 30.1, "zoom.maxPercent": 300}"""
> +        error_message = "Only bool, string, and int values allowed"

Error: local variable 'error_message' is assigned to but never used [flake8: F841]

::: testing/mozbase/mozprofile/tests/test_preferences.py:316
(Diff revision 2)
> +
> +        with mozfile.NamedTemporaryFile(suffix='.json') as f:
> +            f.write(json)
> +            f.flush()
> +
> +            with self.assertRaises(PreferencesReadError) as cm:

Error: local variable 'cm' is assigned to but never used [flake8: F841]
Comment on attachment 8943177 [details]
Bug 1431024 - Modified error criterion check in prefs.py.

https://reviewboard.mozilla.org/r/213516/#review220920

Could you address the reviewbot nits? :) Aside from those this looks pretty good.
Attachment #8943177 - Flags: review?(wlachance) → review-
(In reply to Vedant Chakravadhanula(:vedantc98) from comment #5)
> I've pushed another patch, although I think there might be a few issues.
> For one, I haven't tested the precise error message, only whether or not the
> error is raised.
> The problem is unittest assertRaises, which gives an
> attributeError(__exit__) when it is used without a context manager.

I don't think we need to care about the precise error message tbh. Just testing that the exception is raised should be enough.
Okay, thanks for the feedback William!
I've pushed the latest patch, hopefully this should do it.
Comment on attachment 8943177 [details]
Bug 1431024 - Modified error criterion check in prefs.py.

https://reviewboard.mozilla.org/r/213516/#review220950

::: testing/mozbase/mozprofile/mozprofile/prefs.py:151
(Diff revision 3)
>          elif isinstance(prefs, dict):
>              values = prefs.values()
>          else:
>              raise PreferencesReadError("Malformed preferences: %s" % path)
>          types = (bool, string_types, int)
> -        if [i for i in values if not [isinstance(i, j) for j in types]]:
> +        if [i for i in values if True not in [isinstance(i, j) for j in types]]:

This might be better written as:

[i for i in values if not any([isinstance(i, j) for j in types])]

::: testing/mozbase/mozprofile/tests/test_preferences.py:309
(Diff revision 3)
>  
>              commandline = ["--preferences", f.name]
>              self.compare_generated(_prefs, commandline)
>  
> +    def test_json_datatypes(self):
> +        json = """{"zoom.minPercent": 30.1, "zoom.maxPercent": 300}"""

Could you add a quick comment about why this is malformed?
Attachment #8943177 - Flags: review?(wlachance) → review-
(In reply to Vedant Chakravadhanula(:vedantc98) from comment #10)
> Okay, thanks for the feedback William!
> I've pushed the latest patch, hopefully this should do it.

Just a few small tweaks, we're almost there!
Comment on attachment 8943177 [details]
Bug 1431024 - Modified error criterion check in prefs.py.

https://reviewboard.mozilla.org/r/213516/#review220974


Static analysis found 1 defect in this patch.
 - 1 defect found by mozlint

You can run this analysis locally with:
 - `./mach lint check path/to/file` (Python/Javascript/wpt)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: testing/mozbase/mozprofile/tests/test_preferences.py:309
(Diff revision 4)
>  
>              commandline = ["--preferences", f.name]
>              self.compare_generated(_prefs, commandline)
>  
> +    def test_json_datatypes(self):
> +        #minPercent is at 30.1 to test if non-integer data raises an exception

Error: block comment should start with '# ' [flake8: E265]
Comment on attachment 8943177 [details]
Bug 1431024 - Modified error criterion check in prefs.py.

https://reviewboard.mozilla.org/r/213516/#review220976

Could you fix the new lint warning and schedule a try run similar to the ones we've scheduled for other mozbase modifications? Aside from that this looks great.
Attachment #8943177 - Flags: review?(wlachance) → review-
Comment on attachment 8943177 [details]
Bug 1431024 - Modified error criterion check in prefs.py.

https://reviewboard.mozilla.org/r/213516/#review221452

looks great and try run passes! thanks for your patience on this.
Attachment #8943177 - Flags: review?(wlachance) → review+
Comment on attachment 8943177 [details]
Bug 1431024 - Modified error criterion check in prefs.py.

https://reviewboard.mozilla.org/r/213516/#review221458
vedant: could you mark the issues opened by the code review bot as fixed? apparently I can't land until you do so. :)
Flags: needinfo?(vedantc98)
I've marked them fixed, thanks.
Flags: needinfo?(vedantc98)
Pushed by wlachance@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/a33edd4d8529
Modified error criterion check in prefs.py. r=wlach
https://hg.mozilla.org/mozilla-central/rev/a33edd4d8529
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: