Integrate reftests into build config

RESOLVED FIXED in mozilla33

Status

defect
RESOLVED FIXED
5 years ago
Last year

People

(Reporter: gps, Assigned: gps)

Tracking

(Depends on 2 bugs)

Trunk
mozilla33
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 1 obsolete attachment)

We've long wanted to incorporate reftests into the build config. This will enable things like more efficient packaging.

Patch series will be posted momentarily.
The immediate goal of this patch is to give the build system and testing
tools the knowledge to identify reftest files and directories. Parsing
extra metadata out of reftest manifests is currently a non-requirement,
but may be supported some day.
Attachment #8442244 - Flags: review?(roc)
Attachment #8442244 - Flags: review?(mshal)
Now that we've established a slightly better Python reftest manifest
parser, switch the existing manifest parsing code in
print-manifest-dirs.py to use it.
Attachment #8442245 - Flags: review?(roc)
reftest and crashtest manifests can now be added to the build
configuration via REFTEST_MANIFESTS and CRASHTEST_MANIFESTS,
respectively.

The master manifest files have been added to layout/moz.build.

This patch enables the deprecation of master reftest manifests but stops
short of doing it. In the future, we could declare reftest and crashtest
manifests in their nearest moz.build file and generate the master
manifest (consisting of a bunch of "include" directives) as part of
config.status.
Attachment #8442246 - Flags: review?(mshal)
Attachment #8442246 - Flags: feedback?(roc)
FWIW, the impetus behind this patch was enabling detection of reftests from mach commands. See e.g. bug 920193. A side-effect of adding parsed reftest metadata to the build config is that the all-tests.json file now contains all the reftest/crashtest paths and tools like `mach test` can thus identify reftests (just like they can already identify xpcshell and mochitest).
Comment on attachment 8442244 [details] [diff] [review]
Generic Python code for parsing reftest manifests

>+                # Entire line is a comment.
>+                if line.startswith('#'):
>+                    continue

I think you can remove this block if you make RE_COMMENT '\s*#', since the following lines will strip the line down to nothing, and you already skip empty lines.
Attachment #8442244 - Flags: review?(mshal) → review+
Attachment #8442246 - Flags: review?(mshal) → review+
A push to try revealed that not all directories were getting packaged.
This patch fixes it.

I verified the output of print-manifest-dirs.py before and after is
consistent and the contents of the tests package are identical. I should
have tested more before asking for review.
Attachment #8442961 - Flags: review?(roc)
Attachment #8442244 - Attachment is obsolete: true
Comment on attachment 8442961 [details] [diff] [review]
Generic Python code for parsing reftest manifests

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

We encounter an error in the old print-manifest-dirs.py in bug 1027501 when the reftest contains useless line like

::: layout/tools/reftest/reftest/__init__.py
@@ +77,5 @@
> +                items = line.split()
> +                tests = []
> +
> +                for i in range(len(items)):
> +                    item = items[i]

nit: for i, item in enumerate(items):

@@ +111,5 @@
> +
> +                    if item == '==' or item == '!=':
> +                        tests.extend(items[i+1:i+3])
> +                        break
> +

We encounter an error in the old print-manifest-dirs.py in bug 1027501 when the reftest contains invalidate line like ">>>>". It seems that this new version does not raise an error, but it will ignore them siliently. Is it possible to print an error message or raise an exception while parsing an unexpected syntax?
Duplicate of this bug: 1027501
Comment on attachment 8442961 [details] [diff] [review]
Generic Python code for parsing reftest manifests

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

::: layout/tools/reftest/reftest/__init__.py
@@ +109,5 @@
> +                        tests.append(items[i+1])
> +                        break
> +
> +                    if item == '==' or item == '!=':
> +                        tests.extend(items[i+1:i+3])

Suggest print error message if len(items) < (i + 3) to prevent wrong line syntax 
eg:
"skip-if(B2G) == aaa.html"
I think we should file a follow-up to make the Python parser more strict. I don't want to conflate an apples to apples port with extra modification.
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.