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

RESOLVED FIXED in mozilla28

Status

RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Gijs, Assigned: emorley)

Tracking

unspecified
mozilla28
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

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.
The docs are wrong :-(
(In reply to Ted Mielczarek [:ted.mielczarek] from comment #1)
> The docs are wrong :-(

Alright, changed the summary.
Summary: skip-if = os == mac doesn't work, skip-if = os == "mac" does work → Build documentation about skip-if = os == whatever is wrong (should include quotes around 'whatever')

Comment 3

5 years ago
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

5 years ago
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.
(Assignee)

Comment 5

5 years ago
This almost tripped me up today - are we updating the docs to use quotes or supporting both?
(Assignee)

Updated

5 years ago
OS: Mac OS X → All
Hardware: x86 → All
(Assignee)

Comment 6

5 years ago
Created attachment 823354 [details] [diff] [review]
Fix docs
(Assignee)

Updated

5 years ago
Assignee: nobody → emorley
Status: NEW → ASSIGNED
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.
(Assignee)

Comment 8

5 years ago
(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
https://hg.mozilla.org/mozilla-central/rev/3a33c6c8dfd0
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
You need to log in before you can comment on or make changes to this bug.