Closed Bug 858622 Opened 7 years ago Closed 6 years ago

Make jit-tests runnable on mobile

Categories

(Core :: JavaScript Engine, defect)

ARM
Android
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla26

People

(Reporter: ted, Assigned: dminor)

References

Details

Attachments

(1 file, 2 obsolete files)

We should make the jit-tests harness runnable on mobile, similar to how we have remotexpcshelltests.py.
Duplicate of this bug: 900098
I've been working on similar things for c++ unit tests, I'll take a look at this one as well.
Terrence and I worked on something this week. The patch needs a bit of final polish and testing on a panda, but should be ready early next week.
Assignee: general → dminor
I'm getting a large number (~300) of failures when running the jit-tests on my panda board. I've attached a log file from one of my runs.

As far as I can tell, the paths are correct and no files are missing, but I'm wondering if I'm running tests I should be excluding or if I'm missing some options.
Flags: needinfo?(terrence)
Is there any other output? it looks like all of the output we're getting for most tests is 'r\0Exit code: 0', and exit codes of 0 are usually a good thing.  I'd guess that the issue is either a bit of garbage being printed to stdout, or just reading the wrong return code at some point in time.
I looked at a random failure (bug617485.js), and it appears that it's supposed to fail with a syntax error, which suggests to me that perhaps however you're running the test is not taking this annotation into account:

// |jit-test| error:SyntaxError

That's the first line of the test js file. The corresponding output in the log:

/data/local/tests/js-jit-tests/js -f /data/local/tests/js-jit-tests/jit-tests/lib/prolog.js -e "const platform=\"linux2\"; const libdir=\"/data/local/tests/js-jit-tests/jit-tests/lib/\"; const scriptdir=\"/data/local/tests/js-jit-tests/jit-tests/tests/auto-regress/\"" -f /data/local/tests/js-jit-tests/jit-tests/tests/auto-regress/bug617485.js
/data/local/tests/js-jit-tests/jit-tests/tests/auto-regress/bug617485.js:6:0 SyntaxError: illegal character:
/data/local/tests/js-jit-tests/jit-tests/tests/auto-regress/bug617485.js:6:0 #2=*
/data/local/tests/js-jit-tests/jit-tests/tests/auto-regress/bug617485.js:6:0 ^
^@Exit code: 3
FAIL - /home/dminor/mozilla-central/js/src/jit-test/tests/auto-regress/bug617485.js
Thanks for the pointers. I'll take a look at the check_output function and look for parse issues with data coming back from the device and/or missing test annotations. The majority of the tests are passing, so this hopefully is a small problem.
The problem was that I was only getting stdout from the remote device, but check_output expects the error to be on stderr. I'll work around this.
Status: NEW → ASSIGNED
Flags: needinfo?(terrence)
Filed bug 906525 to fix intermittent failures I hit when running on my pandaboard.
Attached patch bug-858622-fix.patch (obsolete) — Splinter Review
Total running time using the SUT agent is around 20 minutes, including about 3 minutes to push the files to the device. I've left ADB support in, but it is unusably slow due to how the files are pushed to the device under ADB. SUT is what is used in production, but please let me know if we need to make ADB work for local testing and I will file a follow up bug.
Attachment #790894 - Attachment is obsolete: true
Attachment #792169 - Flags: review?(terrence)
Comment on attachment 792169 [details] [diff] [review]
bug-858622-fix.patch

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

Excellent! I did not find any serious issues at all, only a handful of style nits. Lets get this in and running on TBPL.

::: js/src/jit-test/jit_test.py
@@ +74,5 @@
>                    help='Run tests with all IonMonkey option combinations (ignores --jitflags)')
>      op.add_option('-j', '--worker-count', dest='max_jobs', type=int, default=max_jobs_default,
>                    help='Number of tests to run in parallel (default %default)')
> +    op.add_option('--remote', action='store_true',
> +                  help='run tests on remote device')

Lets match the style of the rest of the help text entries: capital letter to start, but no period.

@@ +78,5 @@
> +                  help='run tests on remote device')
> +
> +    op.add_option("--deviceIP", action="store",
> +                    type = "string", dest = "device_ip",
> +                    help = "ip address of remote device to test")

the other entries use foo=bar style.

@@ +84,5 @@
> +    op.add_option("--devicePort", action="store",
> +                    type = int, dest = "device_port", default=20701,
> +                    help = "port of remote device to test")
> +
> +    op.add_option("--dm_trans", action="store",

Could we call this deviceTransport to match the other options?

@@ +89,5 @@
> +                    type = "string", dest = "dm_trans", default = "sut",
> +                    help = "the transport to use to communicate with device: [adb|sut]; default=sut")
> +
> +    op.add_option("--remoteTestRoot", dest='remote_test_root', action = "store",
> +                  type = "string", default='/data/local/tests')

This entry needs a help item.

::: js/src/tests/lib/jittests.py
@@ +22,5 @@
>  
>  from progressbar import ProgressBar, NullProgressBar
>  from results import TestOutput
>  
> +import posixpath

This import should go inline into the import line for os, sys, etc, at the top of the file below the __future__ import.

@@ +293,5 @@
>  
> +def run_test_remote(test, device, prefix, options):
> +    cmd = test.command(prefix, posixpath.join(options.remote_test_root, 'lib/'), posixpath.join(options.remote_test_root, 'tests'))
> +    if options.show_cmd:
> +        print(subprocess.list2cmdline(cmd)) 

Whitespace at end of line.

@@ +303,5 @@
> +    env['LD_LIBRARY_PATH'] = options.remote_test_root
> +
> +    buf = StringIO.StringIO()
> +    returncode = device.shell(cmd, buf, env=env, cwd=options.remote_test_root,
> +                                       timeout=int(options.timeout))

Line up timeout with cmd.

@@ +307,5 @@
> +                                       timeout=int(options.timeout))
> +
> +    out = buf.getvalue()
> +    # can't distinguish between stdout and stderr so we pass same buffer
> +    # to both

Make this a complete sentence with a subject, initial capital, and period.

@@ +572,5 @@
> +    for test in tests:
> +        yield run_test_remote(test, device, prefix, options)
> +
> +def push_libs(options, device):
> +

No space before the start of the function.

@@ +574,5 @@
> +
> +def push_libs(options, device):
> +
> +    # this saves considerable time in pushing unnecessary libraries
> +    # but needs to be maintained if dependencies change

Start with a capital and end with a period.

@@ +602,5 @@
> +        if options.device_ip == None:
> +            print("Error: you must provide a device IP to connect to via the --device option")
> +            sys.exit(1)
> +
> +    #update test root to point to our test dir

Space after #, upgrade to a capital U, and add a period.

@@ +605,5 @@
> +
> +    #update test root to point to our test dir
> +    options.remote_test_root = posixpath.join(options.remote_test_root, 'jit-tests')
> +
> +    # Push js shell and libraries

Add a period.
Attachment #792169 - Flags: review?(terrence) → review+
Thanks for the review. With comments addressed: https://hg.mozilla.org/integration/mozilla-inbound/rev/57ee0ba35de6
Everybody else was happy enough with it, but Windows felt the need to fail 26208 jit-tests, https://tbpl.mozilla.org/php/getParsedLog.php?id=26744603&tree=Mozilla-Inbound&full=1

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/c21468186d3d
Difference from the previous patch:

+        if remote_prefix:
+            # If we are running through the device manager we need to escape the string differently.
+            expr = ('const platform="%s"; const libdir="%s"; const scriptdir="%s"'
+                    % (sys.platform, libdir, scriptdir_var))
+        else:
+            expr = ('const platform=%r; const libdir=%r; const scriptdir=%r'
+                    % (sys.platform, libdir, scriptdir_var))
+

rather than:

+        expr = ('const platform="%s"; const libdir="%s"; const scriptdir="%s"'
+                % (sys.platform, libdir, scriptdir_var))

Admittedly not the most elegant solution. Green try run on Windows here: https://tbpl.mozilla.org/?tree=Try&rev=cea42b3d45c2
Attachment #792169 - Attachment is obsolete: true
Attachment #792896 - Flags: review?(terrence)
Comment on attachment 792896 [details] [diff] [review]
Patch to run jit tests remotely

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

::: js/src/tests/lib/jittests.py
@@ +162,5 @@
>          if not scriptdir_var.endswith('/'):
>              scriptdir_var += '/'
> +
> +        if remote_prefix:
> +            # If we are running through the device manager we need to escape the string differently.

Let's be a bit more specific, so that this doesn't turn into magic later. How about:

# Platforms where subprocess immediately invokes exec do not care whether we use
# double or single quotes. On windows and when using a remote device, however, we
# have to be careful to use the quote style that is the opposite of what the exec
# wrapper uses. This uses %r to get single quotes on windows and special cases
# ADB/SUT, which require double quotes.
fmt = 'const platform=%r; const libdir=%r; const scriptdir=%r'
if remote_prefix:
        fmt = 'const platform="%s"; const libdir="%s"; const scriptdir="%s"'
expr = fmt % (sys.platform, libdir, scriptdir_var))
Attachment #792896 - Flags: review?(terrence) → review+
https://hg.mozilla.org/mozilla-central/rev/da926b6a5368
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
Depends on: 908253
Depends on: 909323
Depends on: 909265
Duplicate of this bug: 601153
You need to log in before you can comment on or make changes to this bug.