Closed Bug 1027215 Opened 7 years ago Closed 7 years ago

Integrate reftests into build config


(Firefox Build System :: General, defect)

Not set


(Not tracked)



(Reporter: gps, Assigned: gps)


(Depends on 2 open bugs)



(3 files, 1 obsolete file)

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 to use it.
Attachment #8442245 - Flags: review?(roc)
reftest and crashtest manifests can now be added to the build

The master manifest files have been added to layout/

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 file and generate the master
manifest (consisting of a bunch of "include" directives) as part of
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 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 in bug 1027501 when the reftest contains useless line like

::: layout/tools/reftest/reftest/
@@ +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 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/
@@ +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 
"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.