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)
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
Comment hidden (mozreview-request) |
Assignee | ||
Updated•6 years ago
|
Assignee: nobody → vedantc98
Assignee | ||
Comment 2•6 years ago
|
||
William, should that be enough to fix this? Thanks.
Comment 3•6 years ago
|
||
mozreview-review |
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-
Comment hidden (mozreview-request) |
Assignee | ||
Comment 5•6 years ago
|
||
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 6•6 years ago
|
||
mozreview-review |
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 7•6 years ago
|
||
mozreview-review |
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-
Comment 8•6 years ago
|
||
(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.
Comment hidden (mozreview-request) |
Assignee | ||
Comment 10•6 years ago
|
||
Okay, thanks for the feedback William! I've pushed the latest patch, hopefully this should do it.
Comment 11•6 years ago
|
||
mozreview-review |
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-
Comment 12•6 years ago
|
||
(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 hidden (mozreview-request) |
Comment 14•6 years ago
|
||
mozreview-review |
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 15•6 years ago
|
||
mozreview-review |
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 hidden (mozreview-request) |
Assignee | ||
Comment 17•6 years ago
|
||
Done, the try build is at https://treeherder.mozilla.org/#/jobs?repo=try&revision=bf37b249aec88b26301a7ca0a3db1d329372bedf
Comment 18•6 years ago
|
||
mozreview-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 19•6 years ago
|
||
mozreview-review |
Comment on attachment 8943177 [details] Bug 1431024 - Modified error criterion check in prefs.py. https://reviewboard.mozilla.org/r/213516/#review221458
Comment 20•6 years ago
|
||
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)
Comment 22•6 years ago
|
||
Pushed by wlachance@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/a33edd4d8529 Modified error criterion check in prefs.py. r=wlach
Comment 23•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/a33edd4d8529
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox60:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
You need to log in
before you can comment on or make changes to this bug.
Description
•