926821 - Build documentation about skip-if = os == whatever is wrong (should include quotes around 'whatever')

Status: RESOLVED FIXED

(Testing :: Mozbase, defect)

Description

[:Gijs (he/him)]

The documentation pages here:

https://ci.mozilla.org/job/mozilla-central-docs/Build_Documentation/test_manifests.html#manifest-destiny-manifests

and here:

https://ci.mozilla.org/job/mozilla-central-docs/Build_Documentation/mozinfo.html#mozinfo-attributes

and here:

http://mozbase.readthedocs.org/en/latest/manifestdestiny.html

and even the comment here:

http://mxr.mozilla.org/mozilla-central/source/testing/mozbase/manifestdestiny/manifestparser/manifestparser.py#680

Seem to indicate that this:

[foo.js]
skip-if = os == mac

Should work in a browser.ini test manifest. It doesn't. You need quotes around the OS, which is also, ironically, what the example in http://mxr.mozilla.org/mozilla-central/source/testing/mozbase/manifestdestiny/manifestparser/manifestparser.py#680 uses.

Please either fix all this documentation (and any existing consumers with broken syntax), or fix the parser so that it works as advertised.

Comment 1

[(not currently active) Ted Mielczarek]

The docs are wrong :-(

Comment 2

[:Gijs (he/him)]

(In reply to Ted Mielczarek [:ted.mielczarek] from comment #1)
> The docs are wrong :-(

Alright, changed the summary.

Comment 3

[Gregory Szorc [:gps]]

Sphinx supports doctest integration (http://sphinx-doc.org/ext/doctest.html). So, we could have Sphinx test our code examples as it builds the docs. I can say from experience that this is an insanely useful practice. These higher-level tests tend to complement lower-level unit tests nicely.

If we did this, there is a stronger case for docs being part of the build. We don't want bad docs to propagate! This would mean importing Sphinx and some other dependencies in the tree. Annoying, but not a deal breaker. This could be a followup, however.

Comment 4

[Jeff Hammel]

FWIW, I am not opposed to making the magic variables, um, magic variables 
(e.g. 'mac = "mac"').

I also have no problem not doing this ;)  

And yes, the docs should be fixed.

Comment 5

[Ed Morley [:emorley]]

This almost tripped me up today - are we updating the docs to use quotes or supporting both?

Comment 6

[Ed Morley [:emorley]]

Attachment: Fix docs

Comment 7

[(not currently active) Ted Mielczarek]

Comment on attachment 823354 [details] [diff] [review]
Fix docs

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

::: build/docs/test_manifests.rst
@@ +62,5 @@
>  There is a special **DEFAULT** section whose keys/metadata apply to all
>  sections/tests::
>  
>      [DEFAULT]
> +    property = "value"

This doesn't need quotes, this is not an expression.

::: testing/mozbase/manifestdestiny/manifestparser/manifestparser.py
@@ -675,5 @@
>  
>      def filter(self, values, tests):
>          """
>          filter on a specific list tag, e.g.:
> -        run-if = os == win linux

Ugh, this must have been a historical accident.

Comment 8

[Ed Morley [:emorley]]

(In reply to Ted Mielczarek [:ted.mielczarek] from comment #7)
> >      [DEFAULT]
> > +    property = "value"
> 
> This doesn't need quotes, this is not an expression.

Oh ha yeah oops.

Fixed that and landed:
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/3a33c6c8dfd0

Comment 9

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/3a33c6c8dfd0