1027215 - Integrate reftests into build config

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Gregory Szorc [:gps]]

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.

Comment 1

[Gregory Szorc [:gps]]

Attachment: Generic Python code for parsing reftest manifests

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.

Comment 2

[Gregory Szorc [:gps]]

Attachment: Rewrite print-manifest-dirs.py to use new manifest parser

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.

Comment 3

[Gregory Szorc [:gps]]

Attachment: Add reftest manifests to build config

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.

Comment 4

[Gregory Szorc [:gps]]

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 5

[Michael Shal [:mshal]]

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.

Comment 6

[Gregory Szorc [:gps]]

Attachment: Generic Python code for parsing reftest manifests

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.

Comment 7

[Ting-Yu Lin [:TYLin] (PST, UTC-7)]

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?

Comment 9

[u459114]

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"

Comment 10

[Gregory Szorc [:gps]]

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.

Comment 11

[Gregory Szorc [:gps]]

https://hg.mozilla.org/integration/fx-team/rev/ffaf206fd018
https://hg.mozilla.org/integration/fx-team/rev/ab997ac497a0
https://hg.mozilla.org/integration/fx-team/rev/f6e2373c5b2a

Comment 12

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/ffaf206fd018
https://hg.mozilla.org/mozilla-central/rev/ab997ac497a0
https://hg.mozilla.org/mozilla-central/rev/f6e2373c5b2a