932552 - Add tests (and documentation) for comments in manifest expressions

Status: RESOLVED FIXED

(Testing :: Mozbase, defect)

Description

[Mark Hammond [:markh] [:mhammond]]

Attachment: 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.

Comment 1

[Jeff Hammel]

(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

[Jeff Hammel]

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!

Comment 3

[(not currently active) Ted Mielczarek]

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

[Jeff Hammel]

(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

[Jeff Hammel]

(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?

Comment 6

[Mark Hammond [:markh] [:mhammond]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/745c8b54f04f

I also opened bug 934304 for the fact that syntax errors are silently swallowed.

Comment 7

[Mark Hammond [:markh] [:mhammond]]

Also added a note to https://developer.mozilla.org/en-US/docs/XPCshell_Test_Manifest_Expressions

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/745c8b54f04f

Comment 9

[Jeff Hammel]

This also needs to be landed on github:

https://github.com/mozilla/mozbase/blob/master/manifestdestiny/tests/test_expressionparser.py

Comment 10

[Jeff Hammel]

pushed to github: https://github.com/mozilla/mozbase/commit/cc12e2c8b5abccd751c91e57c8ff37f5e8cb220c