Closed
Bug 600639
Opened 14 years ago
Closed 14 years ago
Support for running and debugging trace-tests on android
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
RESOLVED
FIXED
People
(Reporter: cjones, Assigned: cjones)
Details
Attachments
(4 files, 3 obsolete files)
3.74 KB,
patch
|
Details | Diff | Splinter Review | |
9.46 KB,
patch
|
dmandelin
:
review+
|
Details | Diff | Splinter Review |
2.66 KB,
patch
|
dmandelin
:
review+
|
Details | Diff | Splinter Review |
1.64 KB,
patch
|
cjones
:
review+
|
Details | Diff | Splinter Review |
Android makes things hard by - closing stdio fds in favor of a non-standard kernel-level logging system - disabling ptrace for non-root programs, which means that |gdbserver js| would have root (no thanks!) - making it hard to run python from a normal shell The upcoming patches hack around this by - allowing overrides of gOutFile and gErrFile and patching trace-test.py to use those - adding a |js -g| option to have it sleep on startup to give time to attach gdbserver - making trace_test.py usable as a module, so that a wrapper script can import it and pass arguments
Assignee | ||
Comment 1•14 years ago
|
||
r? --> sayrer because I don't know who should review these patches. Please reassign if appropriate.
Attachment #479495 -
Flags: review?
Assignee | ||
Comment 2•14 years ago
|
||
Attachment #479496 -
Flags: review?
Comment 3•14 years ago
|
||
Comment on attachment 479495 [details] [diff] [review] Allow overriding gOutFile and gErrFile in js shell, and add a -g option to sleep on startup so that a debugger can be attached I created the Python harness, so I'm probably qualified to review. :-)
Attachment #479495 -
Flags: review? → review?(dmandelin)
Updated•14 years ago
|
Attachment #479496 -
Flags: review? → review?(dmandelin)
Assignee | ||
Comment 4•14 years ago
|
||
Cool, thanks.
Comment 5•14 years ago
|
||
Comment on attachment 479495 [details] [diff] [review] Allow overriding gOutFile and gErrFile in js shell, and add a -g option to sleep on startup so that a debugger can be attached Looks good. Please add documentation for the new features to https://developer.mozilla.org/En/SpiderMonkey/Introduction_to_the_JavaScript_shell Also, I'm wondering if 30 seconds is always the right amount of time to wait, or would it be better to use a numerical option?
Attachment #479495 -
Flags: review?(dmandelin) → review+
Assignee | ||
Comment 6•14 years ago
|
||
Added |-g [seconds]|, doc'd.
Attachment #479495 -
Attachment is obsolete: true
Comment 7•14 years ago
|
||
Comment on attachment 479496 [details] [diff] [review] part 2: Make trace_test.py importable, add support for avoiding stdio with js, and add a generally-useful --write-failure-log option I approve of this shortening: > def run_test(test, lib_dir): >+ env = os.environ.copy() > if test.tmflags: >- env = os.environ.copy() > env['TMFLAGS'] = test.tmflags >- else: >- env = None > cmd = get_test_cmd(test.path, test.jitflags, lib_dir) This request is kind of marginal, but I think run_test gets a little long with this patch, and the interleaving of options is a bit much to share one line of code. How about new functions like "run_cmd" and "run_cmd_avoid_stdio" or the like, with input of a cmdline and env, and output of out, err, and returncode. Also, I think you only need to call decode() for the output of the subprocess, not things read from files. >@@ -143,27 +141,42 @@ def run_test(test, lib_dir): > if os.uname()[0] == 'Darwin': > valgrind_prefix += ['--dsymutil=yes'] > cmd = valgrind_prefix + cmd > > if OPTIONS.show_cmd: > print(subprocess.list2cmdline(cmd)) > # close_fds is not supported on Windows and will cause a ValueError. > close_fds = sys.platform != 'win32' >- p = Popen(cmd, stdin=PIPE, stdout=PIPE, stderr=PIPE, close_fds=close_fds, env=env) >+ if not OPTIONS.avoid_stdio: >+ stdout, stderr = PIPE, PIPE >+ else: >+ fd, stdoutPath = tempfile.mkstemp(prefix='jsstdout') >+ os.close(fd) >+ fd, stderrPath = tempfile.mkstemp(prefix='jsstderr') >+ os.close(fd) >+ env['JS_STDOUT'] = stdoutPath >+ env['JS_STDERR'] = stderrPath >+ stdout, stderr = None, None >+ p = Popen(cmd, stdin=PIPE, stdout=stdout, stderr=stderr, close_fds=close_fds, env=env) > out, err = p.communicate() >+ if OPTIONS.avoid_stdio: >+ out = open(stdoutPath).read() >+ os.unlink(stdoutPath) >+ err = open(stderrPath).read() >+ os.unlink(stderrPath) > out, err = out.decode(), err.decode() > if OPTIONS.show_output: > sys.stdout.write(out) > sys.stdout.write(err) > sys.stdout.write('Exit code: ' + str(p.returncode) + "\n") > if test.valgrind: > sys.stdout.write(err) > return (check_output(out, err, p.returncode, test.allow_oom, test.error), >- out, err) >+ out, err, p.returncode) > The help message is a little coy for my taste. It may even be good to just name the option "--android" and say that it makes it actually work there. Speaking of which, could you use sys.platform to enable that mode by default on Android? Maybe it wouldn't even have to be a command-line option. We try to make the JS test suites "just work" as much as possible. >+ op.add_option('--avoid-stdio', dest='avoid_stdio', action='store_true', >+ help='stdio is broken. Avoid it.') Finally, on the write_output_log thing, it is redundant with some existing features. I'm also thinking of jstests.py, which has a lot of overlap and which we should merge with this harness someday. jstests.py has a -O FILENAME option, which redirects the output from -s and -o (which I think prints the info you want). So I'd prefer to just add the -O option here.
Attachment #479496 -
Flags: review?(dmandelin)
Assignee | ||
Comment 8•14 years ago
|
||
(In reply to comment #7) > Comment on attachment 479496 [details] [diff] [review] > part 2: Make trace_test.py importable, add support for avoiding stdio with js, > and add a generally-useful --write-failure-log option > > This request is kind of marginal, but I think run_test gets a little long with > this patch, and the interleaving of options is a bit much to share one line of > code. How about new functions like "run_cmd" and "run_cmd_avoid_stdio" or the > like, with input of a cmdline and env, and output of out, err, and returncode. > Also, I think you only need to call decode() for the output of the subprocess, > not things read from files. > Done. > The help message is a little coy for my taste. It may even be good to just name > the option "--android" and say that it makes it actually work there. Speaking > of which, could you use sys.platform to enable that mode by default on Android? > Maybe it wouldn't even have to be a command-line option. We try to make the JS > test suites "just work" as much as possible. > Ah, good idea. sys.platform is 'linux2' on android, fortunately or not. However, the python installation we're using on device adds its own |android| module, so I added code to look for that. It's far from foolproof though, but should work well enough. I realized that technically we should be checking to see if stdout/stderr are broken, but it's seems like a bit much for me. > Finally, on the write_output_log thing, it is redundant with some existing > features. I'm also thinking of jstests.py, which has a lot of overlap and which > we should merge with this harness someday. jstests.py has a -O FILENAME option, > which redirects the output from -s and -o (which I think prints the info you > want). So I'd prefer to just add the -O option here. Yes ... none were quite what I wanted, -O included. I split this off into a separate patch, will discuss there.
Assignee: general → jones.chris.g
Attachment #479710 -
Flags: review?
Assignee | ||
Comment 9•14 years ago
|
||
My use case for this flag is - I want a list of failed tests. --write-failures does this already. - but I want to see output from failed tests. --show-output writes it to stdout, and -O to file in jstests.py. - but I want to see output *only* from failed tests. I've found this very useful so far when running trace-test on device. AFAICT there's no combination of existing flags that gives me that.
Attachment #479496 -
Attachment is obsolete: true
Attachment #479711 -
Flags: review?
Assignee | ||
Updated•14 years ago
|
Attachment #479710 -
Flags: review? → review?(dmandelin)
Assignee | ||
Updated•14 years ago
|
Attachment #479711 -
Flags: review? → review?(dmandelin)
Assignee | ||
Comment 10•14 years ago
|
||
Rob, word of warning: I was testing my revised patches on device when the test harness started pausing randomly during -j runs (which are fine wrt js/src ilooping). On some pauses, I saw the js-shell process stuck somewhere in libc, not using any CPU, but I couldn't tell where without symbols. I also saw trace_test.py pausing when trying to read the redirected stdout/stderr files. Tried to debug, found a couple of potential issues, but nothing fixed things. I then tried the original patch from bug 600488 and got the same kinds of random pauses. After rebooting my device, they went away. I suspect something fs related, but don't know what it might be.
Updated•14 years ago
|
Attachment #479710 -
Flags: review?(dmandelin) → review+
Comment 11•14 years ago
|
||
(In reply to comment #9) > Created attachment 479711 [details] [diff] [review] > part 3: Add a --write-failure-log option to trace-test that logs only failed > tests and their output > > My use case for this flag is > - I want a list of failed tests. --write-failures does this already. > - but I want to see output from failed tests. --show-output writes it to > stdout, and -O to file in jstests.py. > - but I want to see output *only* from failed tests. > > I've found this very useful so far when running trace-test on device. AFAICT > there's no combination of existing flags that gives me that. That is a good use case, so let's do it. I still want to clean up the option handling a bit more. How about having 2 options, one to say what to print, the other to say where to print it, kind of like Mochitest. --failure-log=[tests,output] --failure-log-file=[FILENAME] And I'd also like to keep '-w FILENAME' as sugar for --failure=log=tests --failure-log-file=FILENAME. Alternative proposals welcome.
Assignee | ||
Comment 12•14 years ago
|
||
I think that --failure-log=output would imply --failure-log=tests. Maybe the simplest thing is just to have another switch that means "also log output for failed tests", --log-failure-output perhaps. If you want to keep the switch renaming to a minimum, perhaps we could just add a --write-failure-output flag to modify --write-failures?
Comment 13•14 years ago
|
||
(In reply to comment #12) > I think that --failure-log=output would imply --failure-log=tests. I didn't make that explicit, but that's what I was thinking too. > Maybe the > simplest thing is just to have another switch that means "also log output for > failed tests", --log-failure-output perhaps. > > If you want to keep the switch renaming to a minimum, perhaps we could just add > a --write-failure-output flag to modify --write-failures? Sounds good to me.
Assignee | ||
Comment 14•14 years ago
|
||
Attachment #479711 -
Attachment is obsolete: true
Attachment #479860 -
Flags: review?(dmandelin)
Attachment #479711 -
Flags: review?(dmandelin)
Comment 15•14 years ago
|
||
Comment on attachment 479860 [details] [diff] [review] part 3: Add a --write-failure-output option to trace-test to additionally log output from failed tests Beautiful!
Attachment #479860 -
Flags: review?(dmandelin) → review+
Assignee | ||
Comment 16•14 years ago
|
||
http://hg.mozilla.org/tracemonkey/rev/103703dccf3f http://hg.mozilla.org/tracemonkey/rev/b0ad32580f98 http://hg.mozilla.org/tracemonkey/rev/724d72d48b16
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Comment 17•14 years ago
|
||
Gah. I forgot that calling the PR_ functions would break the non-threadsafe shell build. Can we fix it with this patch in the meantime?
Attachment #480269 -
Flags: review?(jones.chris.g)
Assignee | ||
Updated•14 years ago
|
Attachment #480269 -
Flags: review?(jones.chris.g) → review+
Comment 18•14 years ago
|
||
http://hg.mozilla.org/tracemonkey/rev/ec984b89106f
You need to log in
before you can comment on or make changes to this bug.
Description
•