1335796 - Move webidl parser tests out of 'make check'

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Michael Shal [:mshal]]

We can move the webidl parser tests (dom/bindings/parser/runtests.py) out of 'make check' pretty easily. It looks like the tests run fine without running configure or requiring any generated files, so we can run them as a lint task in Taskcluster.

Comment 1

[Michael Shal [:mshal]]

:jgraham, it looks like you've touched this test runner before. Do we need to run the tests on every platform? Or is it sufficient to run them as a linting task just on Linux?

Here's an example try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=6501b07ca43774207226b98916ba3f43a9f7b402&selectedJob=73434073

It's the tc-W(p) job (name is subject to change).

Also of note, I found out that the 'make check' version of these tests regenerates the webidlyacc.py file on every test invocation, so it takes ~3 minutes to run in 'make check' and ~10 seconds to run as 'mach webidl-parser-test'. Obviously we'll be doing the latter in the Taskcluster task.

Comment 2

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

A single platform should be fine, this is just testing a python script that shouldn't depend on the platform.

Comment 3

[Michael Shal [:mshal]]

Attachment: Bug 1335796 - Move WebIDL parser tests out of 'make check';

This test suite involves a bunch of python tests which don't require
configure or any generated binaries to run. We can split them out into a
Taskcluster linter type task to run directly on the source tree in
parallel with builds.

Review commit: https://reviewboard.mozilla.org/r/108800/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/108800/

Comment 4

[Michael Shal [:mshal]]

r?dustin for the task definition to make sure I'm not copy/pasting bad things, and r?Ms2ger for the build system bits. Ms2ger, please also check the files-changed list in the task definition to make sure it's reasonably accurate, and feel free to bikeshed on the treeherder symbol name (currently Wp under lint-opt).

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=3374dea67b6e2aafba8df21748d3789a3ae8a478

Comment 5

[Dustin J. Mitchell [:dustin] (he/him)]

Comment on attachment 8832610 [details]
Bug 1335796 - Move WebIDL parser tests out of 'make check';

https://reviewboard.mozilla.org/r/108800/#review110114

As I understand it, mozlint.yml is for stuff that runs `./mach lint`, which this doesn't.  Just move the stanza to a new file (say, `webidl.yml`) and add that file to `jobs-from` in `kind.yml`.  Other than that, this looks great.

Comment 6

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 8832610 [details]
Bug 1335796 - Move WebIDL parser tests out of 'make check';

https://reviewboard.mozilla.org/r/108800/#review110328

Have we even been checking the results of this test so far? I guess we might be checking the `TEST-UNEXPECTED` output…

::: dom/bindings/parser/runtests.py:109
(Diff revision 1)
>  if __name__ == '__main__':
>      parser = get_parser()
>      args = parser.parse_args()
>      if args.verbose is None:
>          args.verbose = True
>      run_tests(args.tests, verbose=args.verbose)

Maybe also `sys.exit()` with the return value here?

::: taskcluster/ci/source-check/mozlint.yml:118
(Diff revision 1)
> +        - release
> +    when:
> +        files-changed:
> +            - 'dom/bindings/parser/runtests.py'
> +            - 'dom/bindings/parser/WebIDL.py'
> +            - 'dom/bindings/parser/tests/**'

Sounds okay.

Comment 7

[Michael Shal [:mshal]]

(In reply to Dustin J. Mitchell [:dustin] from comment #5)
> As I understand it, mozlint.yml is for stuff that runs `./mach lint`, which
> this doesn't.  Just move the stanza to a new file (say, `webidl.yml`) and
> add that file to `jobs-from` in `kind.yml`.  Other than that, this looks
> great.

Makes sense, thanks! Now it's webidl.yml.

(In reply to :Ms2ger (⌚ UTC+1/+2) from comment #6)
> Have we even been checking the results of this test so far? I guess we might
> be checking the `TEST-UNEXPECTED` output…

Yeah, mozharness scrapes the 'make check' output looking for TEST-UNEXPECTED. So even though 'make check' was returning 0 in this case because of the lack of a return code, mozharness still sets things up so the build turns orange/red (depending on buildbot vs. taskcluster) in treeherder.

> ::: dom/bindings/parser/runtests.py:109
> (Diff revision 1)
> >  if __name__ == '__main__':
> >      parser = get_parser()
> >      args = parser.parse_args()
> >      if args.verbose is None:
> >          args.verbose = True
> >      run_tests(args.tests, verbose=args.verbose)
> 
> Maybe also `sys.exit()` with the return value here?

Good idea, I added this. I also added a sys.path.append() similar to the mach command so the webidlyacc.py result is cached when running runtests.py manually (so we get the 10s runtime instead of 3m).

Comment 8

[Michael Shal [:mshal]]

Comment on attachment 8832610 [details]
Bug 1335796 - Move WebIDL parser tests out of 'make check';

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/108800/diff/1-2/

Comment 9

[Dustin J. Mitchell [:dustin] (he/him)]

Comment on attachment 8832610 [details]
Bug 1335796 - Move WebIDL parser tests out of 'make check';

https://reviewboard.mozilla.org/r/108800/#review110404

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7e3a47f5a91a
Move WebIDL parser tests out of 'make check'; r=dustin,Ms2ger

Comment 11

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/7e3a47f5a91a