Closed
Bug 932552
Opened 11 years ago
Closed 11 years ago
Add tests (and documentation) for comments in manifest expressions
Categories
(Testing :: Mozbase, defect)
Testing
Mozbase
Tracking
(Not tracked)
RESOLVED
FIXED
mozilla28
People
(Reporter: markh, Assigned: markh)
Details
Attachments
(1 file)
(deleted),
patch
|
k0scist
:
review+
|
Details | Diff | Splinter Review |
Via bug 932147 comment 2, there is some confusion about whether comments work in manifest expressions (ie, on the same line as an expression).
It turns out this does work, somewhat by accident - the _tokenize method passes a list of regexes to the re module's scanner method. The line is parsed until no further matches can be found in the string, and the rest of the string is silently ignored. The '#' character doesn't match any of the regexes, so tokenizing stops at that character and the rest of the line is ignored.
Seeing this is somewhat accidental but still a very worthwhile feature, there should be tests to ensure they continue to work - that way, if the code is ever refactored or reimplemented, the test will tell the developer if special treatment needs to be given to comment characters.
Also, if this is accepted, I'll add a note to https://developer.mozilla.org/en-US/docs/XPCshell_Test_Manifest_Expressions to developers are explicitly aware of this feature.
Attachment #824342 -
Flags: review?(jhammel)
Comment 1•11 years ago
|
||
(In reply to Mark Hammond [:markh] from comment #0)
> Created attachment 824342 [details] [diff] [review]
> Add tests to demonstrate comments work.
>
> Via bug 932147 comment 2, there is some confusion about whether comments
> work in manifest expressions (ie, on the same line as an expression).
>
> It turns out this does work, somewhat by accident - the _tokenize method
> passes a list of regexes to the re module's scanner method. The line is
> parsed until no further matches can be found in the string, and the rest of
> the string is silently ignored. The '#' character doesn't match any of the
> regexes, so tokenizing stops at that character and the rest of the line is
> ignored.
>
> Seeing this is somewhat accidental but still a very worthwhile feature,
> there should be tests to ensure they continue to work - that way, if the
> code is ever refactored or reimplemented, the test will tell the developer
> if special treatment needs to be given to comment characters.
>
> Also, if this is accepted, I'll add a note to
> https://developer.mozilla.org/en-US/docs/XPCshell_Test_Manifest_Expressions
> to developers are explicitly aware of this feature.
Nice det(In reply to Mark Hammond [:markh] from comment #0)
> Created attachment 824342 [details] [diff] [review]
> Add tests to demonstrate comments work.
>
> Via bug 932147 comment 2, there is some confusion about whether comments
> work in manifest expressions (ie, on the same line as an expression).
>
> It turns out this does work, somewhat by accident - the _tokenize method
> passes a list of regexes to the re module's scanner method. The line is
> parsed until no further matches can be found in the string, and the rest of
> the string is silently ignored. The '#' character doesn't match any of the
> regexes, so tokenizing stops at that character and the rest of the line is
> ignored.
>
> Seeing this is somewhat accidental but still a very worthwhile feature,
> there should be tests to ensure they continue to work - that way, if the
> code is ever refactored or reimplemented, the test will tell the developer
> if special treatment needs to be given to comment characters.
>
> Also, if this is accepted, I'll add a note to
> https://developer.mozilla.org/en-US/docs/XPCshell_Test_Manifest_Expressions
> to developers are explicitly aware of this feature.
Nice detective work. IMHO, the current behaviour, while good for comments, is....somewhat awful, since effectively syntax errors are ignored. But that can be dealt with later.
Comment 2•11 years ago
|
||
Comment on attachment 824342 [details] [diff] [review]
Add tests to demonstrate comments work.
Review of attachment 824342 [details] [diff] [review]:
-----------------------------------------------------------------
::: testing/mozbase/manifestdestiny/tests/test_expressionparser.py
@@ +69,5 @@
> + # detail - the '#' character doesn't match any of the regular
> + # expressions we specify as tokens, and thus are ignored.
> + # However, having explicit tests for them means that should the
> + # implementation ever change, comments continue to work, even if that
> + # means a new implementation must handle them explicitly.
Please turn this into a python docstring. Otherwise great work and thanks for the patch!
Attachment #824342 -
Flags: review?(jhammel) → review+
Comment 3•11 years ago
|
||
We discussed this same thing in bug 922581, where some Mozmill manifests were abusing this to stick extra data at the end of the expression (but not a comment). I really think we should fix the parser to error in these situations, but I'd be in favor of keeping end-of-line comments working in any event.
Comment 4•11 years ago
|
||
(In reply to Ted Mielczarek [:ted.mielczarek] from comment #3)
> We discussed this same thing in bug 922581, where some Mozmill manifests
> were abusing this to stick extra data at the end of the expression (but not
> a comment). I really think we should fix the parser to error in these
> situations, but I'd be in favor of keeping end-of-line comments working in
> any event.
+1, my thoughts exactly
Comment 5•11 years ago
|
||
(In reply to Jeff Hammel [:jhammel] from comment #2)
> Comment on attachment 824342 [details] [diff] [review]
> Add tests to demonstrate comments work.
>
> Review of attachment 824342 [details] [diff] [review]:
> -----------------------------------------------------------------
>
> ::: testing/mozbase/manifestdestiny/tests/test_expressionparser.py
> @@ +69,5 @@
> > + # detail - the '#' character doesn't match any of the regular
> > + # expressions we specify as tokens, and thus are ignored.
> > + # However, having explicit tests for them means that should the
> > + # implementation ever change, comments continue to work, even if that
> > + # means a new implementation must handle them explicitly.
>
> Please turn this into a python docstring. Otherwise great work and thanks
> for the patch!
Want me to land or do you have permissions?
Updated•11 years ago
|
Flags: needinfo?(mhammond)
Assignee | ||
Comment 6•11 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/745c8b54f04f
I also opened bug 934304 for the fact that syntax errors are silently swallowed.
Assignee | ||
Comment 7•11 years ago
|
||
Also added a note to https://developer.mozilla.org/en-US/docs/XPCshell_Test_Manifest_Expressions
Assignee: nobody → mhammond
Status: NEW → ASSIGNED
Flags: needinfo?(mhammond)
Comment 8•11 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
Comment 9•11 years ago
|
||
This also needs to be landed on github:
https://github.com/mozilla/mozbase/blob/master/manifestdestiny/tests/test_expressionparser.py
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment 10•11 years ago
|
||
pushed to github: https://github.com/mozilla/mozbase/commit/cc12e2c8b5abccd751c91e57c8ff37f5e8cb220c
Status: REOPENED → RESOLVED
Closed: 11 years ago → 11 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•