Closed Bug 638219 Opened 14 years ago Closed 10 months ago

Integrate jit-test and jstests shell harnesses

Categories

(Core :: JavaScript Engine, enhancement, P3)

Product:
Core
Component:
JavaScript Engine
Type:
enhancement

Tracking

()

Status:
RESOLVED INACTIVE
Milestone:
mozilla21

People

(Reporter: dmandelin, Unassigned)

References

Details

Attachments

(7 files, 29 obsolete files)

Batch 2, patch 1: move command construction into Test class
3.34 KB, patch
djc
: review+
Details | Diff | Splinter Review
Batch 2, patch 2: extract valgrind setup, prepend in Test.command() (v2)
2.50 KB, patch
terrence
: review+
Details | Diff | Splinter Review
Batch 2, patch 3: save static paths in module-level constants (v2)
10.32 KB, patch
djc
: review+
Details | Diff | Splinter Review
Batch 2, patch 4: use TestOutput class to wrap test results
4.69 KB, patch
terrence
: review+
Details | Diff | Splinter Review
Batch 2, patch 5: reuse cmd saved in results object rather than rebuilding it
3.40 KB, patch
terrence
: review+
Details | Diff | Splinter Review
Batch 2, patch 6: move test result output into result processing function
1.23 KB, patch
terrence
: review+
Details | Diff | Splinter Review
Batch 2, patch 7: set all test-independent command-line bits up once, pass in
5.99 KB, patch
terrence
: review+
Details | Diff | Splinter Review
Let's integrate these into one harness that has all the features of either.
I agree we should do this. One thing to watch out for is that jstests currently takes ages, and jit-tests don't. I think we want to keep a "pretty-fast" (20s or so) set of tests or test config. An advantage of combining them is that we can run them as part of |make check|, which means they would test the more esoteric JITFLAGS, and also be run on the spidermonkey-only builds. Oh, and they'd get coverage from |make valgrind-check| which I believe they currently don't.
(In reply to comment #1) > Oh, and they'd get coverage from |make > valgrind-check| which I believe they currently don't. In case you don't already know this: only three of the jit-tests get run under Valgrind: [ocean:~/moz/ws7/js/src/jit-test] rgrep "jit-test.*valgrind" ./tests/basic/testShiftLeft.js:// |jit-test| TMFLAGS: full,fragprofile,treevis; valgrind ./tests/basic/testRegExpTest.js:// |jit-test| TMFLAGS: full,fragprofile,treevis; valgrind ./tests/basic/testSideExitInConstructor.js:// |jit-test| TMFLAGS: full,fragprofile,treevis; valgrind Better than nothing, but not amazing.
(In reply to comment #2) > (In reply to comment #1) > > Oh, and they'd get coverage from |make > > valgrind-check| which I believe they currently don't. > > In case you don't already know this: only three of the jit-tests get run under > Valgrind: > > Better than nothing, but not amazing. Ah, I thought that |make check-valgrind| ran all the tests under valgrind. That is less cool, definitely.
(In reply to comment #3) > > Ah, I thought that |make check-valgrind| ran all the tests under valgrind. That would be sloooooooow...
Bug 530953 comment 1 has an analysis of the command line option differences between the two scripts. Also note that the output formats are different. For trace-tests.py it's like this: [n_run | n_failed | n_passed] for jstests.py it's like this: [n_run | n_failed | n_skipped]
Whiteboard: [mentor=pbiggar@mozilla.com]
Attached patch Move jit tests into tests/ (obsolete) — — Splinter Review
I'm thinking I might be able to move this along a little. The attached patch seems like a sane first step, though it's of course a little bit painful.
(In reply to comment #7) > Created attachment 532712 [details] [diff] [review] [review] > Move jit tests into tests/ > > I'm thinking I might be able to move this along a little. The attached patch > seems like a sane first step, though it's of course a little bit painful. Does that actually work? The tests under tests/ and jit-tests/ have are run with different command lines (and also are discovered using different mechanisms).
No, that obviously didn't work, as I forgot to make it a git diff. This seems to work, though; it passes make check-jit-test for me.
Attachment #532712 - Attachment is obsolete: true
Attached file move jit-tests, non-patch attachment (obsolete) —
Attachment #532891 - Attachment is obsolete: true
Okay, so apparently Bugzilla mangles patches if you check the patch thingy. The latest attachment is unmangled, please have a look.
Scratch that. New proposed approach: keep jit_test.py and jstests.py and just refactor them under the hood and then at some point once the infra is sufficiently unified you can switch everyone to using The One True Way, which could be either of the two ways or a new third way. So, next patch will be something moving all the Python code to one place, while preserving the interface of jit_test.py in the current js/src/jit-test/jit_test.py. If anyone thinks down this path lies madness, please speak up.
Here's an attempt at moving the code for jit_test.py into the tests directory, without changing the interface to jit_test.py. The purpose of this is being able to merge the test code without disturbing the current interface, so that users will only have to change their habits once.
(In reply to comment #12) > Scratch that. New proposed approach: keep jit_test.py and jstests.py and > just refactor them under the hood and then at some point once the infra is > sufficiently unified you can switch everyone to using The One True Way, > which could be either of the two ways or a new third way. I think that's exactly the right approach. The things that are necessarily different between the two are: (a) how to discover the list of tests, (b) how to run a test, and (c) how to interpret the outcome. Stuff like filtering test names, running in multiple threads, and reporting output is what we want to make common. I like your idea of refactoring incrementally instead of trying to fix it all at once (which is pretty much what I initially imagined).
Here's a bundle with my initial progress. Most of what's been done is moving jit_test.py interface onto the more elaborate jstests infrastructure. As part of that, jstests gained a few small features. I've tried to make nicely reviewable chunks of patch, cleaning up the coding style a little bit here and there where I was touching the code anyway. I've run some basic tests, but nothing extensive.
Attachment #532892 - Attachment is obsolete: true
Attachment #532947 - Attachment is obsolete: true
(In reply to comment #15) > Created attachment 533275 [details] > Bundle of 22 small patches for early review > > Here's a bundle with my initial progress. Most of what's been done is moving > jit_test.py interface onto the more elaborate jstests infrastructure. As > part of that, jstests gained a few small features. I've tried to make nicely > reviewable chunks of patch, cleaning up the coding style a little bit here > and there where I was touching the code anyway. I've run some basic tests, > but nothing extensive. Your refactorings are really nice. This looks like a good direction to go, and I don't see any problems with anything in the bundle. We'll have to see how it looks closer when it's done of course, but I expect no problems based on your current progress.
Attachment #533275 - Attachment is patch: true
Attachment #533275 - Attachment mime type: application/octet-stream → text/plain
Attachment #533275 - Attachment is patch: false
Would it be possible to hold off on things like bug 634090 while I hack on this? The patch that got landed for that yesterday basically just copied more code from jstests into jit_test, mostly stuff that I have already factored out in my patch queue, causing massive conflicts in my queue... I think the current state of my patch queue actually already has almost all of the features landed in that patch, so maybe we should just try to start to land the first part of it.
Attached file Fresh bundle, now at 62 patches (obsolete) —
Here's a new bundle, which goes a long way towards full unification. A bunch of options got ported from one to the other or vice versa. Patched marked with -xxx slightly change the functionality/interface, so should get extra review.
Attachment #533275 - Attachment is obsolete: true
djc, how goes this? I've been holding off some test harness changes so you could get this done.
Attached file New bundle, 64 small patches (obsolete) —
A new bundle, with patches to add back features from the backout.
Attachment #535086 - Attachment is obsolete: true
Attached patch Single large patch (obsolete) — — Splinter Review
Here's a single large patch. It should probably be reviewed via the patch queue bundle, because it's hard to read on its own. There are further things that can be done, but I think we should try to land this now to prevent further churn. At least all the really redundant bits have been unified.
Attachment #540343 - Flags: review?(pbiggar)
(In reply to comment #21) > Created attachment 540343 [details] [diff] [review] [review] > Single large patch > > Here's a single large patch. It should probably be reviewed via the patch > queue bundle, because it's hard to read on its own. There are further things > that can be done, but I think we should try to land this now to prevent > further churn. At least all the really redundant bits have been unified. I briefly looked at this and what I saw looked pretty good. One question I have is about the rationale for unifying Test and TestCase in tests.py: I intentionally had those two classes, not because I knew it was really important or anything, but because it seemed that "a program that can be run" and "a test case, i.e., instructions for running a program and interpreting the results" were separate concepts. Did you unify them to solve a particular problem, or for another design reason?
Thanks for the comments. I unified the classes because I felt that the added abstraction didn't make any of the code easier; it felt kind of YAGNI to me. I understand the difference, but I feel like the concept of a "test case" can easily represent both "instructions for running a program" and "interpreting the results". In fact, judging from the code in jstests and jit_test, it seemed to me like in fact the instructions and the way the results should be interpreted are somewhat tightly coupled (in the sense that different test drivers have different kinds of instructions *and* different kinds of results interpretation), so that using a single class seemed to make for less complexity.
I'm having trouble running `jit-test.py -h`. Is this broken? The patch looks good, but I want to test it.
What exactly are you running? python jit-test/jit_test.py -h works for me.
Comment on attachment 540343 [details] [diff] [review] Single large patch Review of attachment 540343 [details] [diff] [review]: ----------------------------------------------------------------- Looking through the patch Q, everything looks right (that is, I think the code is good quality). There appear to be some bugs though: - Missing `#!/usr/bin/env python` in jit_test.py - No args doesn't work: $ jit-test/jit_test.py objdir.DBG/js No tests found matching command line arguments. - JITtests no longer work: $ jit-test/jit_test.py objdir.DBG/js a [ 0| 14| 14] 100% ===============================================>| 0.9s^C REGRESSIONS: - -s flag doesn't work: $ jit-test/jit_test.py objdir.DBG/js args-vargc.js -s /Users/pbiggar/work/mozilla/reviews/js/src/jit-test/jit_test.py -e "const platform='darwin'; const libdir='/Users/pbiggar/work/mozilla/reviews/js/src/jit-test/lib/';" -f /Users/pbiggar/work/mozilla/reviews/js/src/jit-test/lib/prolog.js -m -p -j -f /Users/pbiggar/work/mozilla/reviews/js/src/jit-test/tests/arguments/args-vargc.js Exception in thread Thread-1: Traceback (most recent call last): File "/usr/local/Cellar/python/2.7/lib/python2.7/threading.py", line 530, in __bootstrap_inner self.run() File "/usr/local/Cellar/python/2.7/lib/python2.7/threading.py", line 483, in run self.__target(*self.__args, **self.__kwargs) File "/Users/pbiggar/work/mozilla/reviews/js/src/tests/workers.py", line 83, in run self.sink.push((task, result)) File "/Users/pbiggar/work/mozilla/reviews/js/src/tests/workers.py", line 47, in push self.results.push(result) File "/Users/pbiggar/work/mozilla/reviews/js/src/tests/jittests.py", line 182, in push self.pb.update(self.n, tuple(self.counts)) TypeError: update() takes exactly 2 arguments (3 given) REGRESSIONS: -m -j -p /Users/pbiggar/work/mozilla/reviews/js/src/jit-test/tests/arguments/args-vargc.js -- -o flag doesn't work: $ jit-test/jit_test.py objdir.DBG/js args-vargc.js -o Usage: jit_test.py [options] JS_SHELL [TESTS] jit_test.py: error: no such option: -e Exit code: 2 [ 0| 1| 1] 100% ===============================================>| 0.2s REGRESSIONS: -m -j -p /Users/pbiggar/work/mozilla/reviews/js/src/jit-test/tests/arguments/args-vargc.js -- I think the above problems were caused by my adding the hashbang. But without it I get different problems: $ python jit-test/jit_test.py objdir.DBG/js args-vargc.js Exception in thread Thread-2: Traceback (most recent call last): File "/usr/local/Cellar/python/2.7/lib/python2.7/threading.py", line 530, in __bootstrap_inner self.run() File "/usr/local/Cellar/python/2.7/lib/python2.7/threading.py", line 483, in run self.__target(*self.__args, **self.__kwargs) File "/Users/pbiggar/work/mozilla/reviews/js/src/tests/tests.py", line 100, in th_run_cmd p = Popen(cmd, env=env, **POPTS) File "/usr/local/Cellar/python/2.7/lib/python2.7/subprocess.py", line 672, in __init__ errread, errwrite) File "/usr/local/Cellar/python/2.7/lib/python2.7/subprocess.py", line 1201, in _execute_child raise child_exception OSError: [Errno 8] Exec format error Exception in thread Thread-1: Traceback (most recent call last): File "/usr/local/Cellar/python/2.7/lib/python2.7/threading.py", line 530, in __bootstrap_inner self.run() File "/usr/local/Cellar/python/2.7/lib/python2.7/threading.py", line 483, in run self.__target(*self.__args, **self.__kwargs) File "/Users/pbiggar/work/mozilla/reviews/js/src/tests/workers.py", line 81, in run result = task() File "/Users/pbiggar/work/mozilla/reviews/js/src/tests/tests.py", line 150, in __call__ return self.run(self.prefix, timeout, nostdio) File "/Users/pbiggar/work/mozilla/reviews/js/src/tests/jittests.py", line 109, in run out, err, code, dt, to = tests.run_cmd(cmd, env, timeout, avoid_stdio) File "/Users/pbiggar/work/mozilla/reviews/js/src/tests/tests.py", line 134, in run_cmd return l[1] + (timed_out,) TypeError: unsupported operand type(s) for +: 'NoneType' and 'tuple' 100% ===============================================>| 0.0s PASSED ALL
Attachment #540343 - Flags: review?(pbiggar) → review-
Attached file Updated bundle to address bugs (obsolete) —
Attachment #540342 - Attachment is obsolete: true
Attached patch Updated patch to address bugs (obsolete) — — Splinter Review
Here's an updated patch. It fixes two problems: - Added the hashbang to the new jit_test.py script - Updated the progress bar shim to conform to the progress bar interface This now works with your tests shown above (and a few others).
Attachment #540343 - Attachment is obsolete: true
Attachment #546294 - Flags: review?(pbiggar)
Comment on attachment 546294 [details] [diff] [review] Updated patch to address bugs Review of attachment 546294 [details] [diff] [review]: ----------------------------------------------------------------- The --valgrind-all option doesn't seem to work. I can't see a difference in whether I run --no-slow or not. There's a weird traceback when the js shell is missing: `jit-test/jit_test.py obj` I haven't been testing jstests.py, might want to verify it still works too. Can you say how confident you are that everything still works? ::: js/src/jit-test/jit_test.py @@ +7,2 @@ > > +import jittests self.slow isn't initialized here. Intentional?
Attachment #546294 - Flags: review?(pbiggar) → review-
--valgrind-all works for me when I have valgrind installed. I found a jit-test that has the slow annotation (bug538615.js) and it will not be executed when passed with --no-slow, so that seems to work fine. And the traceback when the shell is missing was weird before my patches, too. I'd be happy to make it nicer, but that wasn't really the purpose of this bug. Not sure what you are referring to with the "self.slow"; you show a hunk at module-level, where self isn't usually defined. If you mean that tests/jittests.py:JITTest.__init__() doesn't define slow, that's correct: it now inherits from tests/tests.py:Test, and self.slow is initialized there. I think the subset of things that still work is probably very large, but it's pretty hard to check all the option interactions. And it's been a while since I actually made the changes... still, I think all the small patches should make it easy to catch most problems, so I think we'd have noticed them sooner. I definitely think the simple cases still work, and I'm happy to be on standby and fix issues as they come up once this lands. I definitely don't think there are any large issues lurking in there.
(In reply to comment #30) > --valgrind-all works for me when I have valgrind installed. My bad, my valgrind installation was broken, and the valgrind exe was missing. Now that it's reinstalled, it works great. > I found a > jit-test that has the slow annotation (bug538615.js) and it will not be > executed when passed with --no-slow, so that seems to work fine. I tested by looking at the test count with --no-slow, and without it, and the count was the same. I think bug 538615 is not run when --no-slow is omitted. > And the > traceback when the shell is missing was weird before my patches, too. I'd be > happy to make it nicer, but that wasn't really the purpose of this bug. Yeah, that's fair enough. > Not sure what you are referring to with the "self.slow"; you show a hunk at > module-level, where self isn't usually defined. If you mean that > tests/jittests.py:JITTest.__init__() doesn't define slow, that's correct: it > now inherits from tests/tests.py:Test, and self.slow is initialized there. Um, weird. I've heard splinter (the new bugzilla review system) does such things. I was aiming at JITTest::__init__. (You're right, I missed the inheritance, so we're good there.) > I think the subset of things that still work is probably very large, but > it's pretty hard to check all the option interactions. And it's been a while > since I actually made the changes... still, I think all the small patches > should make it easy to catch most problems, so I think we'd have noticed > them sooner. I definitely think the simple cases still work, and I'm happy > to be on standby and fix issues as they come up once this lands. I > definitely don't think there are any large issues lurking in there. The small patches make it easier to see what's changing in each one, but it's still difficult to verify the changes. I've been manually testing it, and it seems to me that everything still works, with the objection of the --no-slow thing above. I'll be happy to r+ once that's fixed :)
As for the --no-slow thing, let's just say I carefully preserved the semantics of --no-slow before my patches. ;) --no-slow is internally named run_slow, the default value is None and passing in the option sets it to False... I guess you can figure out what it'll change about the process. Do I have enough reason to feel confident about my patch yet? :)
Comment on attachment 546294 [details] [diff] [review] Updated patch to address bugs Oh yeah, so it is. Looks good then, thanks for all the work on this. Do you need this pushed?
Attachment #546294 - Flags: review- → review+
Oh, one more bug: when I run 'make check-jit-test', I get an error, due to jit-test.py haing an exit code of 2. That will make the builds go orange, so that needs to be fixed :)
I know where that bug comes from and have started to fix it, but I'm leaving for two weeks tomorrow. I'll try to complete the fix soon after. In the meantime, if you want to make progress on hacking jit-test or jstests, perhaps you can work on top of the patch queue? The fix for the return code shouldn't ripple too much.
Assignee: general → dirkjan
Try run for 33b540f49141 is complete. Detailed breakdown of the results available here: http://tbpl.mozilla.org/?tree=Try&rev=33b540f49141 Results: success: 15 failure: 2 Total buildrequests: 17 Builds available at http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/pbiggar@mozilla.com-33b540f49141
Attached patch Fixes exit code problem (obsolete) — — Splinter Review
Attachment #546294 - Attachment is obsolete: true
The build errors in comment 36 are on android (with the attachement from comment 37). There is no log for |make check|, but it fails after "maybe reboot", so I'm guessing something either times out, or it sees no output and doesn't like that. I guess the best way to fix this is to try it on an android machine, so doing that now.
Whiteboard: [mentor=pbiggar@mozilla.com]
Terrence, sfink and I were not sure if jstests were run by the js shell (they are run by the browser, though). If it wasn't, this bug will fix it.
FWIW, single threaded timings from my MBP last night: real 18m49.003s user 8m39.230s sys 4m18.158s I reckon we could achieve wall time of < 3 minutes on this machine (assuming 5x speedup from 4 physical cores + hyperthreading and enough parallel processes to soak up CPU cycles lost from tests not exercising all available CPU cycles). This would be about 15 minutes wall time savings! This would make |make check| run at least 2x faster. Our Linux and OS X builder times are hovering around 45 minutes to an hour (http://brasstacks.mozilla.com/gofaster/#/executiontime/build). If my math is correct, running jit_test.py in parallel would enable us to complete builds on buildbot 20-33% faster! I'm pretty sure RelEng would love if this landed.
The patches here no longer apply. I'm going to take a crack at modernizing them.
I actually worked at that a few months ago, but I didn't get much buy-in from, well, anyone. I still feel kind of annoyed that all of this stuff has bitrotted.
Whoops, my bad, after not seeing movement on this bug for a year, I assumed you had abandoned your patches. Apparently that's not the case! Do the patches that you re-worked still apply, or have they bitrotted as well?
(In reply to Dirkjan Ochtman (:djc) from comment #44) > I actually worked at that a few months ago, but I didn't get much buy-in > from, well, anyone. I still feel kind of annoyed that all of this stuff has > bitrotted. I'm all for supporting this change, though.
I could probably take another whack at it if there are people willing to review and land it. It would be nice if we could two or three patches landed before doing the rest of it. IIRC the first patch in my series was moving most of the jittest code, and that was very prone to generating hard-to-resolve conflicts if anyone wanted to do anything in there (which actually happened a few times during the days I worked on my patch series).
Warning: this is probably going to end up as a bit of a novella, so go grab a hot drink. Also, feel free to skip this if you are not specifically interested in the js test suites. (In reply to Dirkjan Ochtman (:djc) from comment #47) > I could probably take another whack at it if there are people willing to > review and land it. It would be nice if we could two or three patches landed > before doing the rest of it. IIRC the first patch in my series was moving > most of the jittest code, and that was very prone to generating > hard-to-resolve conflicts if anyone wanted to do anything in there (which > actually happened a few times during the days I worked on my patch series). That was just before I got here -- I've always wondered what happened to those patches. Over the last year or so, I've been slowly plinking away at cleaning up the test suites: see bug 745230, bug 745237, bug 746836, and their linked blocker bugs. The keys to working successfully on the js test suites have been: (1) work in small chunks so that our chronically-overworked, non-python team will make time for reviewing non-mission-critical patches and (2) tie each piece of work to a new feature so that you can get buy-in for the review. Until we are at the top of arewefastyet.com, abstract goals like unification and cleanliness are going to get totally ignored, for good reason. That said, there are a huge number of ways that having a better test suite can get us to the top of AWFY faster, we just have to stay focused on providing tangible, incremental benefits. The current state of affairs: jit_test.py will run tests in the shell *only* and /not/ in the browser. It looks for and parses |jit-test| headers at the top of each test file. A test is considered successful if it finishes without an exception or segfault. It supports a bunch of CLI options that are useful specifically to JIT development and which the JIT development team changes the meaning of at-will. It has good support for running tests on ARM. It also has some neat features for storing and re-running tests that Marty uses extensively. It runs test in serial. jstests.py is able to run tests in the shell /and/ its tests can also be run by the browser in "reftest" mode. The makefiles use jstests.py to generate the manifest immediately before starting the tests: there is no longer a clunky physical manifest to update when adding a test. It looks for and parses |reftest| headers: these correspond exactly to the browser's reftest manifest entries, so we cannot change the format. Each test must print a SUCCESS after it runs or the harness will consider it a failure. It runs tests in parallel. The test suites are in a much better state now than they were a year ago. Although I haven't used Dirkjan's patches directly, I've managed to sneak in most of his refactoring work in one form or another. For example, the progressbar code is already shared by the two suites. Observations: - The jit-test suite is much better from a developer perspective: you can just cat a fuzzer testcase into the tree and you are done. Reftests need at least one additional assertion to be added. - The reftest suite requires the shell to work before you can even run tests with it (to parse the more complex |reftest| annotations). - Except for the point above, the reftest suite is much better when actually running shell tests because of the parallelism. - |jit-test| headers and |reftest| headers are just wildly and completely different. Just being able to understand both formats isn't enough: they each need different support from the test runner, from the result parser, and even from the system itself for the valgrind flag. - Some reftest tests require browser features and just nop in the shell. - Most jit-test tests would probably not get any (additional) benefit out of running in the browser, although they wouldn't lose any benefit if moved to the browser. - Moving the jit-tests to the browser on tbpl would allow the build step to complete even faster and allow the jit-tests to run in parallel with more things, but would be very hard to implement. - As Gregory just pointed out we desperately, desperately need the jit-tests to run in parallel. I've been meaning to spend a weekend on this last point and actually managed to make some progress on it two Fridays ago by making jit-tests import and use the jstests progressbar. With this done, the nice parallel test runner implementation in tests/lib/tasks_unix.py is at least available to the jit-tests, in theory. The first step is to make jit-tests use the Test and TestCase classes from tests/lib/tests.py so that it will be compatible with the task runner. I'm not sure what else will be required, but I don't imagine there will be too much more to do after that. This would be a huge win. If it winds up in my review queue, I will make reviewing it my top priority.
Target Milestone: --- → mozilla14
Version: Trunk → 16 Branch
Target Milestone: mozilla14 → ---
Version: 16 Branch → Trunk
Terrence: thanks for the explanation. I fully understand how the JS engine arms race makes things like refactoring test driver scripts not a priority. I was just grumpy because (a) this was my first good chance at contributing to mozilla-central, after lots of ways of contributing in other ways around the fringes, (b) it was a mentored bug, so it was supposed to be easier for a non-core contributor to get something through the process, until the mentor left Mozilla and no one was able to pick it up, and (c) I spent quite a few hours on what was, in my opinion, a pretty high-quality, easily-reviewable patch set, that in the end mostly bitrotted, while bits and pieces were reimplemented by other people from scratch. I would like to try to get something into your review queue this week.
Terrence -- are you going to be available for review during your vacation? If not, you might want to update your bugzilla name with the time range you'll be gone. Terrence is really the right person for the reviews since he has the clearest vision of where this should go (and specifically the obstacles in the way, in the form of what workflow breaks with a given change.) But if he's unavailable, I'll do them.
(In reply to Dirkjan Ochtman (:djc) from comment #49) > I would like to try to get something into your review queue this week. Awesome! I will do my best to make sure we don't drop the ball again. As Steve says, I am out of the office until the end of the year, so do please account for a higher than normal review latency. I will have my machine with me for the duration, and I will try to check my mail and review queue at least daily.
Just FWIW, (In reply to Terrence Cole [:terrence] (on holiday until 2013) from comment #48) > - Moving the jit-tests to the browser on tbpl would allow the build step to > complete even faster and allow the jit-tests to run in parallel with more > things, but would be very hard to implement. I don't think there's any need to move jit-tests to run in-browser. We *should* move them to run on the test machines from the test package, since we could also then run them on Android devices fairly easily, but we can continue running them in the JS shell. We run xpcshell tests similarly, there's no reason we can't do it for JS tests.
I agree with Ted that we should move these tests out of |make check| and into a new test "job." However, I think that's considerably more work than refactoring jit_test.py to run tests in parallel. It's also more difficult to make changes to things once they live as a separate test job. Therefore, I recommend that the course of action be 1) refactor jit_test.py to run in parallel 2) pull things out of |make check|. #2 should be a separate bug blocked upon completion of this one.
> 2) pull things out of |make check|. #2 should be a separate bug > blocked upon completion of this one. I filed bug 822394.
Making a separate test job also involves packaging up everything needed for the tests. Apparently this is already done for jstests, given that they run in the browser in a separate job already? I would think it would go something like 1. merge jit_test and jstests, with |make check| and jsreftests both running at least as much as they currently do 2. perhaps it'll fall out naturally from the above, but get parallel running to work in the merged version (jstests already run in parallel) 3. make the jit_tests packagable for a separate job run (and ensure that the jstests packaging will work for a separate shell-only run in addition to its current in-browser run) 4. create a separate job for running shell tests on the packaged version 5. Remove make check from the build job Once the functionality is there, I can help with the buildbot and tbpl portions of this. It would be very nice to see movement on this; it really bothers me that all of our builds have to wait on |make check| before either reporting status or spawning the dependent test jobs.
(In reply to Steve Fink [:sfink] from comment #55) > Once the functionality is there, I can help with the buildbot and tbpl > portions of this. It would be very nice to see movement on this; it really > bothers me that all of our builds have to wait on |make check| before either > reporting status or spawning the dependent test jobs. nit: "make check" doesn't block the test jobs from starting. Builds are uploaded after the build+package is complete, and then make check is run and other tests are spawned in parallel. Making "make check" faster just means we use up fewer resources on the builders, trading it for time on the test machines. (However, we can run in parallel with other test suites.)
I didn't know that! Ok, that makes me feel better, thanks.
> nit: "make check" doesn't block the test jobs from starting. Builds are > uploaded after the build+package is complete, and then make check is run and > other tests are spawned in parallel. Making "make check" faster just means > we use up fewer resources on the builders, trading it for time on the test > machines. (However, we can run in parallel with other test suites.) However make check does delay the time before we can see if the job passed/failed and also view the log (for the vast majority of people who do not have BuildVPN access and thus are unable to view the stdio on the buildbot master directly). This in the past has impacted the length of time taken for bisecting weird bustage, and therefore the length of time the tree has had to remain closed.
(In reply to Steve Fink [:sfink] from comment #55) > 1. merge jit_test and jstests, with |make check| and jsreftests both running > at least as much as they currently do I'd rather put this at the end of the list. Given how incredibly hard that is going to be, I don't think we should block the important stuff below. Besides, I think this is more likely to fall out of the below than vice-versa. > 2. perhaps it'll fall out naturally from the above, but get parallel running > to work in the merged version (jstests already run in parallel) We're already sharing the ProgressBar. All we need to do is share the definition of a Test and TestCase, which I think Dirkjan is going to be working on this week. > 3. make the jit_tests packagable for a separate job run (and ensure that the > jstests packaging will work for a separate shell-only run in addition to its > current in-browser run) > > 4. create a separate job for running shell tests on the packaged version > > 5. Remove make check from the build job We could even work on 3-5 in parallel to 2 if anyone has the time.
Sorry for being slow on this. Hope to still do it, though, this weekend.
I'd like to see if we can land this patch before I go on doing much else. Last time around, it proved that this kind of patch is way too easily corrupted by other people landing some small fix into the test scripts. I'm not entirely sure why the normpath() didn't work; I tried running from the src directory, with ./jit-test/jit_test.py ./js, and it failed without the replacement of normpath() by abspath().
Attachment #546293 - Attachment is obsolete: true
Attachment #549169 - Attachment is obsolete: true
Attachment #701600 - Flags: review?(terrence)
Did you do a standalone JS build in the srcdir? Note that the "js" binary is actually built into js/src/shell, and symlinked back up to js/src (as well as $(DIST)/bin).
I built using "make" from js/src.
(In reply to Ted Mielczarek [:ted.mielczarek] from comment #63) > Did you do a standalone JS build in the srcdir? Note that the "js" binary is > actually built into js/src/shell, and symlinked back up to js/src (as well > as $(DIST)/bin). I've also hit this error. How does realpath behave in this case?
Sorry, that was absurdly unclear, so let me clarify: people frequently, as part of their normal workflow, run the test suites from the /craziest/ places in their path. We must to support this. My thinking was that we would move each piece of jit-tests over to the tests/lib as needed, but the way you are doing this will work fine too. I think we can even do it without having to re-solve the problem above, since we already have robust code to handle this in add_libdir_to_path. What I'd like to do is: 1) Copy jit-test/jit_tests.py to tests/lib/jittests.py. 2) In jit-test/jit_tests.py, remove everything but |add_libdir_to_path| and |main|. 3) In tests/lib/jittest.py, remove |add_libdir_to_path| (it's already in the path) and |main|. 4) In jit-test/jit_tests.py, call |add_libdir_to_path()| so that we have access to jittests.py, then import all the symbols main uses from jittests.py. This uses our existing search code, so we won't break any existing uses and keeps the driver bits nicely separated from library code (or at least code that will become library code later on).
Here's take 2: - Moves code to tests/lib rather than just tests - Keeps using the add_libdir_to_path() function However, main() is still shipped of to the jittests library with the rest of the code for now, due to somewhat pervasive use of module-level globals. This will be on the agenda for cleanup at a later stage.
Attachment #701600 - Attachment is obsolete: true
Attachment #701600 - Flags: review?(terrence)
Attachment #701920 - Flags: review?(terrence)
Comment on attachment 701920 [details] [diff] [review] Move jit_test code js/src/tests/lib (take 2) Review of attachment 701920 [details] [diff] [review]: ----------------------------------------------------------------- This looks much better. \o/
Attachment #701920 - Flags: review?(terrence) → review+
Try looks good, I think we can land this.
Attached patch follow up (obsolete) — — Splinter Review
It looks like the above patch stripped the execute bit on jit_tests.py. This re-adds it.
Attachment #702445 - Flags: review?(choller)
Comment on attachment 702445 [details] [diff] [review] follow up Whoops, set the reviewer incorrectly.
Attachment #702445 - Flags: review?(choller) → review?(dirkjan)
Comment on attachment 702445 [details] [diff] [review] follow up Ah, yeah, that's a bit stupid.
Attachment #702445 - Flags: review?(dirkjan) → review+
https://hg.mozilla.org/mozilla-central/rev/9de04b80715f https://hg.mozilla.org/mozilla-central/rev/919b82e39cf8
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attachment #701920 - Attachment is obsolete: true
Attachment #702445 - Attachment is obsolete: true
Attachment #705078 - Flags: review?(terrence)
Attachment #705080 - Flags: review?(terrence)
This concludes the first batch. Together, these pass both all of the jit tests, and the jstests results are unchanged (3 regressions). If the reviews are positive, we could land these as a single patch, unless we want to preserve the smaller patches to facilitate post-facto review.
Attachment #705086 - Flags: review?(terrence)
Comment on attachment 705078 [details] [diff] [review] Batch 1, patch 1: remove +x bit from tests/lib/jittests.py Ah, so that's where that bit wondered off to.
Attachment #705078 - Flags: review?(terrence) → review+
Attachment #705079 - Flags: review?(terrence) → review+
Attachment #705080 - Flags: review?(terrence) → review+
Comment on attachment 705083 [details] [diff] [review] Batch 1, patch 4: style: print is a statement, add space around formatting operator Review of attachment 705083 [details] [diff] [review]: ----------------------------------------------------------------- I'd like two changes to this patch: 1) We'd like to move to Python3, which has a print function, so we should be doing |from __future__ import print_function| and keeping the parentheses. Ms2ger did this for the jstests in Bug 832750, although it appears to have yet to land. 2) Use |str.format()| for all string formatting. ::: js/src/tests/lib/jittests.py @@ +440,4 @@ > written.add(test.path) > out.close() > except IOError: > + sys.stderr.write("Exception thrown trying to write failure file '%s'\n" % Please use print(..., file=sys.stderr).
Attachment #705083 - Flags: review?(terrence)
Comment on attachment 705084 [details] [diff] [review] Batch 1, patch 5: pass jit-test script options around as an argument Review of attachment 705084 [details] [diff] [review]: ----------------------------------------------------------------- Very nice. ::: js/src/tests/lib/jittests.py @@ +616,4 @@ > 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)') > > + OPTIONS, args = op.parse_args(argv) OPTIONS isn't a global anymore, so it would probably be better for this to be lower case |options|.
Attachment #705084 - Flags: review?(terrence) → review+
Comment on attachment 705085 [details] [diff] [review] Batch 1, patch 6: pass path to JS shell around as an argument Review of attachment 705085 [details] [diff] [review]: ----------------------------------------------------------------- We should probably do the same thing that the jstests.py does for this: stick |js| on the |options| object as |options.js_shell| so that we only have to pass one object everywhere. This should make the diff quite a bit smaller as well. The relevant code from jstests is in tests/jstests.py on line 134 to 139, although the |requested_paths| manipulation is not relevant to jit-tests. ::: js/src/tests/lib/jittests.py @@ +137,4 @@ > ans.append(test) > return ans > > +def get_test_cmd(js, path, jitflags, lib_dir, shell_args): This can stay: you'll just want to call it as get_test_cmd(options.js_shell, ...).
Attachment #705085 - Flags: review?(terrence)
Comment on attachment 705086 [details] [diff] [review] Batch 1, patch 7: move jittests.main() function back into jit_test.py script Review of attachment 705086 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jit-test/jit_test.py @@ +17,5 @@ > import jittests > > +def main(argv): > + > + script_path = os.path.abspath(sys.modules['__main__'].__file__) You shouldn't need to look in sys.modules anymore to get to __file__.
Attachment #705086 - Flags: review?(terrence) → review+
Thanks for the quick and thorough reviews! (In reply to Terrence Cole [:terrence] from comment #85) > I'd like two changes to this patch: > 1) We'd like to move to Python3, which has a print function, so we should be > doing |from __future__ import print_function| and keeping the parentheses. > Ms2ger did this for the jstests in Bug 832750, although it appears to have > yet to land. > 2) Use |str.format()| for all string formatting. Respectfully, I disagree on these. First, I think I'm still in the cleanup phase for this whole thing, and moving the Python code closer to something that's more recognizable as (PEP8-like) Python is a good thing. What you're talking about is future-proofing at best, and I'd like to save that for some later time when the codebase is in better shape. Second, while a move to Python 3 at some point would be a good thing, I disagree that it's a good goal to start introducing partial Python 3-isms at this point. I'm pretty sure many Python developers at this point aren't too familiar with the look of Python 3, and this goes even more for Mozilla devs for whom Python isn't (even close to) their primary language. Third, I think the jury is still out on whether the str.format() idiom is always the preferred one even in Python 3, and I don't think it's a win for these simple cases (also see the point above about familiarity). So I'd prefer to keep this patch (mostly, see below) as is for now, and we can future-proofing it can be done at some later stage. > ::: js/src/tests/lib/jittests.py > @@ +440,4 @@ > > written.add(test.path) > > out.close() > > except IOError: > > + sys.stderr.write("Exception thrown trying to write failure file '%s'\n" % > > Please use print(..., file=sys.stderr). Given my little rant above, this could become print >> sys.stderr, .... (In reply to Terrence Cole [:terrence] from comment #86) > OPTIONS isn't a global anymore, so it would probably be better for this to > be lower case |options|. Makes sense. (In reply to Terrence Cole [:terrence] from comment #87) > We should probably do the same thing that the jstests.py does for this: > stick |js| on the |options| object as |options.js_shell| so that we only > have to pass one object everywhere. This should make the diff quite a bit > smaller as well. The relevant code from jstests is in tests/jstests.py on > line 134 to 139, although the |requested_paths| manipulation is not relevant > to jit-tests. Yeah, I was thinking about that, will do. (In reply to Terrence Cole [:terrence] from comment #88) > ::: js/src/jit-test/jit_test.py > @@ +17,5 @@ > > import jittests > > > > +def main(argv): > > + > > + script_path = os.path.abspath(sys.modules['__main__'].__file__) > > You shouldn't need to look in sys.modules anymore to get to __file__. Right, good catch.
> Second, while a move to Python 3 at some point would be a good thing, I > disagree that it's a good goal to start introducing partial Python 3-isms at > this point. I'm pretty sure many Python developers at this point aren't too > familiar with the look of Python 3, and this goes even more for Mozilla devs > for whom Python isn't (even close to) their primary language. Speaking as someone who's only started learning Python in the last month, it seems worthwhile using the Python 3 forms that should be valid forever, rather than the Python 2 forms that won't.
(In reply to Dirkjan Ochtman (:djc) from comment #89) > Thanks for the quick and thorough reviews! > > (In reply to Terrence Cole [:terrence] from comment #85) > > I'd like two changes to this patch: > > 1) We'd like to move to Python3, which has a print function, so we should be > > doing |from __future__ import print_function| and keeping the parentheses. > > Ms2ger did this for the jstests in Bug 832750, although it appears to have > > yet to land. > > 2) Use |str.format()| for all string formatting. > > Respectfully, I disagree on these. First, I think I'm still in the cleanup > phase for this whole thing, and moving the Python code closer to something > that's more recognizable as (PEP8-like) Python is a good thing. What you're > talking about is future-proofing at best, and I'd like to save that for some > later time when the codebase is in better shape. > > Second, while a move to Python 3 at some point would be a good thing, I > disagree that it's a good goal to start introducing partial Python 3-isms at > this point. I'm pretty sure many Python developers at this point aren't too > familiar with the look of Python 3, and this goes even more for Mozilla devs > for whom Python isn't (even close to) their primary language. > > Third, I think the jury is still out on whether the str.format() idiom is > always the preferred one even in Python 3, and I don't think it's a win for > these simple cases (also see the point above about familiarity). > > So I'd prefer to keep this patch (mostly, see below) as is for now, and we > can future-proofing it can be done at some later stage. I don't care about str.format() vs %-style, but I strongly object to moving away from python 3-compat instead of towards it. Python 2 is a dead end, so getting people more used to python 3'isms like print-as-a-function is unambiguously good thing, IMO. Adding more technical debt isn't.
(In reply to :Ms2ger from comment #91) > I don't care about str.format() vs %-style, but I strongly object to moving > away from python 3-compat instead of towards it. Python 2 is a dead end, so > getting people more used to python 3'isms like print-as-a-function is > unambiguously good thing, IMO. Adding more technical debt isn't. I strongly agree with Ms2ger on all these points.
Review optional (:terrence r+'ed v1).
Attachment #705084 - Attachment is obsolete: true
Attachment #705085 - Attachment is obsolete: true
Attachment #705407 - Flags: review?(terrence)
No review necessary (:terrence +1ed v1).
Attachment #705086 - Attachment is obsolete: true
Comment on attachment 705407 [details] [diff] [review] Batch 1, patch 6: pass path to JS shell around as an option value Review of attachment 705407 [details] [diff] [review]: ----------------------------------------------------------------- Perfect!
Attachment #705407 - Flags: review?(terrence) → review+
This is a proposed alternative to patch 4. It requires a minimum of python2.6 (for the __future__.print_function import).
Attachment #706442 - Flags: review?(terrence)
> This is a proposed alternative to patch 4. It requires a minimum of > python2.6 (for the __future__.print_function import). We already require Python 2.7 to build Firefox.
Comment on attachment 706442 [details] [diff] [review] Batch 1, patch 4: style: treat print as a function, improve formatting Review of attachment 706442 [details] [diff] [review]: ----------------------------------------------------------------- Thank you! I think that's just what's called for here.
Attachment #706442 - Flags: review?(terrence) → review+
(In reply to Gary Kwong [:gkw] from comment #98) > > This is a proposed alternative to patch 4. It requires a minimum of > > python2.6 (for the __future__.print_function import). > > We already require Python 2.7 to build Firefox. Though not to test it.
(In reply to :Ms2ger from comment #100) > (In reply to Gary Kwong [:gkw] from comment #98) > > > This is a proposed alternative to patch 4. It requires a minimum of > > > python2.6 (for the __future__.print_function import). > > > > We already require Python 2.7 to build Firefox. > > Though not to test it. While we have some test runners still running <2.7, the move to require 2.7 applies for the test runners as well. i.e. the test runners can be upgraded to and made to require 2.7 without any future policy change or announcement. As long as tests pass on official infra and 2.7, you are good to go.
We only currently run jit-tests as part of "make check" on the build machines anyway, so Python 2.7 is required.
Attachment #705083 - Attachment is obsolete: true
The try results look good to me, can someone verify, please?
Sorry I haven't gotten around to pushing this yet, I'm in major crunch time for GGC.
Thanks for letting me know, I'll wait a bit longer. :)
Attachment #705078 - Flags: checkin+
Attachment #705079 - Flags: checkin+
Attachment #705080 - Flags: checkin+
Attachment #705406 - Flags: checkin+
Attachment #705407 - Flags: checkin+
Attachment #705409 - Flags: checkin+
Attachment #706442 - Flags: checkin+
Attachment #705406 - Flags: review+
Attachment #705409 - Flags: review+
(In reply to Dirkjan Ochtman (:djc) from comment #106) > Thanks for letting me know, I'll wait a bit longer. :) Done! Steve still had this sitting in his patch queue, so I asked him to push to m-i. What do you have planned next?
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
I'll have a look at next steps on Monday, probably.
Attachment #705078 - Attachment is obsolete: true
Attachment #705079 - Attachment is obsolete: true
Attachment #705080 - Attachment is obsolete: true
Attachment #705406 - Attachment is obsolete: true
Attachment #705407 - Attachment is obsolete: true
Attachment #705409 - Attachment is obsolete: true
Attachment #706442 - Attachment is obsolete: true
Attachment #713472 - Flags: review?(terrence)
Comment on attachment 713472 [details] [diff] [review] Batch 2, patch 1: move command construction into Test class Review of attachment 713472 [details] [diff] [review]: ----------------------------------------------------------------- That's a nice cleanup!
Attachment #713472 - Flags: review?(terrence) → review+
Comment on attachment 713473 [details] [diff] [review] Batch 2, patch 2: extract valgrind setup, prepend in Test.command() Review of attachment 713473 [details] [diff] [review]: ----------------------------------------------------------------- This needs a bit more work to remove the global usage. Ask me on IRC if what I've requested isn't clear. ::: js/src/tests/lib/jittests.py @@ +29,5 @@ > + 'valgrind', '-q', '--smc-check=all-non-file', '--error-exitcode=1', > + '--gen-suppressions=all', '--show-possibly-lost=no', '--leak-check=full' > + ] > + if os.uname()[0] == 'Darwin': > + VALGRIND.append('--dsymutil=yes') I like how you've cached the valgrind command bits, rather than recomputing them every test, but we should store the cached value as a class property on Test, not in the globals. It turns out it's really easy to do this in python. The |class| statement creates a new |dict|, sets it as the global, runs the code in the class's definition, then sets that dict as the new class object's __dict__ property. Thus /any/ definitions you make in the class definition will be accessible through the class and shared by all objects of the class, not just method definitions. For example: ~~~~~~ class Foo: tmp = 'bar' Bar = '' if tmp: Bar = tmp del tmp def show_bar(self): print(self.Bar) def show_tmp(self): print(self.tmp) Foo().show_bar() # prints 'bar' Foo().show_tmp() # AttributeError: no 'tmp' in class Foo ~~~~~~ Please use this to scope the valgrind bits to Test. Also please rename VALGRIND to ValgrindCommand. Likewise, call _PATHS paths and _VALGRINDS valgrinds and |del| them when you are done defining ValgrindCommand. @@ +145,5 @@ > # via the "|jit-test|" line. Remove dups because they are toggles. > + cmd = [js] + list(set(self.jitflags)) + shell_args + ['-e', expr] > + cmd += ['-f', os.path.join(lib_dir, 'prolog.js'), '-f', self.path] > + if self.valgrind: > + cmd = VALGRIND + cmd Then you can access the valgrind bits as |self.ValgrindCommand| here.
Attachment #713473 - Flags: review?(terrence)
Comment on attachment 713474 [details] [diff] [review] Batch 2, patch 3: save static paths in module-level constants Review of attachment 713474 [details] [diff] [review]: ----------------------------------------------------------------- This is a good use of globals. ::: js/src/jit-test/jit_test.py @@ +107,5 @@ > read_all = False > try: > f = open(options.read_tests) > for line in f: > + test_list.append(os.path.join(TEST_DIR, line.strip('\n'))) I think this needs to be jittests.TEST_DIR. ::: js/src/tests/lib/jittests.py @@ +142,2 @@ > if not libdir_var.endswith('/'): > libdir_var += '/' Since we fully control LIB_DIR, would it make sense to just assert that LIB_DIR endswith '/' and use it directly?
Attachment #713474 - Flags: review?(terrence) → review+
Comment on attachment 713472 [details] [diff] [review] Batch 2, patch 1: move command construction into Test class Review of attachment 713472 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/tests/lib/jittests.py @@ +241,1 @@ > Also, please remove this extra space.
(In reply to Terrence Cole [:terrence] from comment #115) > Comment on attachment 713473 [details] [diff] [review] > Batch 2, patch 2: extract valgrind setup, prepend in Test.command() > > Review of attachment 713473 [details] [diff] [review]: > ----------------------------------------------------------------- > > This needs a bit more work to remove the global usage. Ask me on IRC if what > I've requested isn't clear. > > ::: js/src/tests/lib/jittests.py > @@ +29,5 @@ > > + 'valgrind', '-q', '--smc-check=all-non-file', '--error-exitcode=1', > > + '--gen-suppressions=all', '--show-possibly-lost=no', '--leak-check=full' > > + ] > > + if os.uname()[0] == 'Darwin': > > + VALGRIND.append('--dsymutil=yes') > > I like how you've cached the valgrind command bits, rather than recomputing > them every test, but we should store the cached value as a class property on > Test, not in the globals. What's the value of this? Having it as a class attribute is mostly equivalent to having it as a module-level constant, and there is nothing Test-specific about the valgrind we use. I'd see some value if we could replace the Test.valgrind instance property directly with the command, but since it seems to rely on test flags, that won't work. > Please use this to scope the valgrind bits to Test. Also please rename > VALGRIND to ValgrindCommand. Likewise, call _PATHS paths and _VALGRINDS > valgrinds and |del| them when you are done defining ValgrindCommand. I also think ValgrindCommand is a bad name. Capitalized names are generally reserved for classes only in common Python style; it'd have to be valgrind_command to comply with PEP8-like style.
(In reply to Terrence Cole [:terrence] from comment #116) > Comment on attachment 713474 [details] [diff] [review] > Batch 2, patch 3: save static paths in module-level constants > > Review of attachment 713474 [details] [diff] [review]: > ----------------------------------------------------------------- > > This is a good use of globals. > > ::: js/src/jit-test/jit_test.py > @@ +107,5 @@ > > read_all = False > > try: > > f = open(options.read_tests) > > for line in f: > > + test_list.append(os.path.join(TEST_DIR, line.strip('\n'))) > > I think this needs to be jittests.TEST_DIR. Right, will fix that. > ::: js/src/tests/lib/jittests.py > @@ +142,2 @@ > > if not libdir_var.endswith('/'): > > libdir_var += '/' > > Since we fully control LIB_DIR, would it make sense to just assert that > LIB_DIR endswith '/' and use it directly? I think Test.command() is the only user of LIB_DIR, so I'd say we don't even need the assertion if we just make sure LIB_DIR ends with a '/'.
(In reply to Dirkjan Ochtman (:djc) from comment #118) > (In reply to Terrence Cole [:terrence] from comment #115) > > Comment on attachment 713473 [details] [diff] [review] > > Batch 2, patch 2: extract valgrind setup, prepend in Test.command() > > > > Review of attachment 713473 [details] [diff] [review]: > > ----------------------------------------------------------------- > > > > This needs a bit more work to remove the global usage. Ask me on IRC if what > > I've requested isn't clear. > > > > ::: js/src/tests/lib/jittests.py > > @@ +29,5 @@ > > > + 'valgrind', '-q', '--smc-check=all-non-file', '--error-exitcode=1', > > > + '--gen-suppressions=all', '--show-possibly-lost=no', '--leak-check=full' > > > + ] > > > + if os.uname()[0] == 'Darwin': > > > + VALGRIND.append('--dsymutil=yes') > > > > I like how you've cached the valgrind command bits, rather than recomputing > > them every test, but we should store the cached value as a class property on > > Test, not in the globals. > > What's the value of this? Having it as a class attribute is mostly > equivalent to having it as a module-level constant, and there is nothing > Test-specific about the valgrind we use. I'd see some value if we could > replace the Test.valgrind instance property directly with the command, but > since it seems to rely on test flags, that won't work. Yeah, there is a bit of history here. Ideally, we'd run all tests under valgrind, but we can't because it's just too slow -- I think it takes several weeks actually. The fuzzers do run this test occasionally, but it's resource intensive so they don't do it often. What they do instead is fuzz the shell with small tests under valgrind. When the fuzzers find a test that fails under valgrind they add the valgrind annotation to force the test to run in the same environment that they used to find it. I think there are only a handful of tests with this flag and I think in practice they don't actually run on tinderbox because of the valgrind requirement. Your patch is a nice enhancement to the jit-test suite because currently we have no way to know when a failing test was running under valgrind without knowing that the test has the annotation. To answer your actual question, there are two reasons that I'd like to see VALGRIND scoped to Test. First, Test is the sole user, so scoping the definition to Test makes it clear that changing the definition of VALGRIND will only effect Test. More importantly, it forces the definition to stay inside Test. We don't want it getting forgotten or worse, duplicated, when we transplant the Test code into a different file. > > Please use this to scope the valgrind bits to Test. Also please rename > > VALGRIND to ValgrindCommand. Likewise, call _PATHS paths and _VALGRINDS > > valgrinds and |del| them when you are done defining ValgrindCommand. > > I also think ValgrindCommand is a bad name. Capitalized names are generally > reserved for classes only in common Python style; it'd have to be > valgrind_command to comply with PEP8-like style. Yeah, that's a good point: I was thinking in SpiderMonkey style, but this should be PEP8. I requested a name change mostly because I'd like to separate it from the |valgrind| flag by more than simple capitalization. Would you be okay with VALGRIND_COMMAND?
> When the fuzzers find a test that > fails under valgrind they add the valgrind annotation to force the test to > run in the same environment that they used to find it. I think there are > only a handful of tests with this flag and I think in practice they don't > actually run on tinderbox because of the valgrind requirement. I think it's only three jit-tests that get run under Valgrind (and I know that testShiftLeft.js is one of them). I always run jit-tests under Valgrind and periodically one fails because of Valgrind finding an error. But jit-tests.py doesn't indicate that the failure is because of Valgrind and so the failing command line that it reports doesn't actually fail, and I have to manually add the valgrind flags.
(In reply to Nicholas Nethercote [:njn] from comment #121) > I think it's only three jit-tests that get run under Valgrind (and I know > that testShiftLeft.js is one of them). I always run jit-tests under > Valgrind and periodically one fails because of Valgrind finding an error. > But jit-tests.py doesn't indicate that the failure is because of Valgrind > and so the failing command line that it reports doesn't actually fail, and I > have to manually add the valgrind flags. I think this bug will be fixing that problem: it attaches the valgrind bits to the command line in Test itself, rather than tacking them on just before the run.
> I think this bug will be fixing that problem: it attaches the valgrind bits > to the command line in Test itself, rather than tacking them on just before > the run. Excellent! (That's what I hoped, but I didn't quite understand from your comment if it would happen.)
Removed the newline, although under protest! Man, you're nitpicky... Can a man keep a tiny little bit of his personal style intact around here? ;) I always start functions with an empty line if the rest of the function contains multiple vertical blocks. By not making the function header stick to the first block, the blocks are more clearly delineated.
Attachment #713472 - Attachment is obsolete: true
Attachment #714276 - Flags: review+
Okay, your analysis makes some sense. I've used VALGRIND_CMD for the name; it's a little shorter, and should clarify and distinguish enough from other uses.
Attachment #713473 - Attachment is obsolete: true
Attachment #714277 - Flags: review?(terrence)
Added a trailing slash to LIB_DIR. Extended the changeset message a little bit to explain why I think it's okay to point to directory paths that are somewhat far removed from the script, since that feels a little bit ugly to me.
Attachment #713474 - Attachment is obsolete: true
Attachment #714280 - Flags: review+
Missed some signatures in the last version, this is better.
Attachment #714280 - Attachment is obsolete: true
Attachment #714287 - Flags: review+
In particular, not passing shell_args around as much will help later on...
Attachment #714416 - Flags: review?(terrence)
Reporting on test output seems a better fit for process_test_results() than for run_test().
Attachment #714420 - Flags: review?(terrence)
I think that pretty much concludes batch 2. In the next batch, I can start to move closer to using the jstests tasks framework for the JIT tests.
Comment on attachment 714375 [details] [diff] [review] Batch 2, patch 4: use TestOutput class to wrap test results Review of attachment 714375 [details] [diff] [review]: ----------------------------------------------------------------- That's a very nice cleanup!
Attachment #714375 - Flags: review?(terrence) → review+
Attachment #714416 - Flags: review?(terrence) → review+
Comment on attachment 714420 [details] [diff] [review] Batch 2, patch 6: move test result output into result processing function Review of attachment 714420 [details] [diff] [review]: ----------------------------------------------------------------- Agreed!
Attachment #714420 - Flags: review?(terrence) → review+
Comment on attachment 714423 [details] [diff] [review] Batch 2, patch 7: set all test-independent command-line bits up once, pass in Review of attachment 714423 [details] [diff] [review]: ----------------------------------------------------------------- Sorry I didn't get to these sooner!
Attachment #714423 - Flags: review?(terrence) → review+
(In reply to Terrence Cole [:terrence] from comment #135) > Sorry I didn't get to these sooner! No problem. I think you forgot to take a look at the new patch 2, though!
Comment on attachment 714277 [details] [diff] [review] Batch 2, patch 2: extract valgrind setup, prepend in Test.command() (v2) Review of attachment 714277 [details] [diff] [review]: ----------------------------------------------------------------- Doh! You are right, I missed this one somehow. This version looks very nice.
Attachment #714277 - Flags: review?(terrence) → review+
Since my push privileges still haven't been fixed (bug 837533), can someone please bundle up these changes and push them into try?
Try run for 00f6759b085f is complete. Detailed breakdown of the results available here: https://tbpl.mozilla.org/?tree=Try&rev=00f6759b085f Results (out of 45 total builds): success: 42 warnings: 1 failure: 2 Builds (or logs if builds failed) available at: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/Ms2ger@gmail.com-00f6759b085f
Try run for 00f6759b085f is complete. Detailed breakdown of the results available here: https://tbpl.mozilla.org/?tree=Try&rev=00f6759b085f Results (out of 46 total builds): success: 42 warnings: 1 failure: 3 Builds (or logs if builds failed) available at: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/Ms2ger@gmail.com-00f6759b085f
So I think the try results look good. Can someone look them over to make sure I didn't miss anything and if I didn't, push the patches out to inbound?
Yeah, looks good. I'll push as soon as m-i opens.
Comment on attachment 714375 [details] [diff] [review] Batch 2, patch 4: use TestOutput class to wrap test results Review of attachment 714375 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/tests/lib/jittests.py @@ +498,2 @@ > else: > lines = [ _ for _ in out.split('\n') + err.split('\n') So, yeah, you forgot to change these, which means that a test failure now turns red and shows 'command timed out: 3600 seconds without output, attempting to kill'; see <https://tbpl.mozilla.org/php/getParsedLog.php?id=20245776&tree=Mozilla-Inbound> for example.
Thanks for fixing that up. Are there any best practices I can follow to make sure I catch these kinds of bugs before stuff is pushed?
Looks like pyflakes <https://pypi.python.org/pypi/pyflakes> caught it: $ pyflakes js/src/tests/lib/jittests.py js/src/tests/lib/jittests.py:17: 'cpu_count' imported but unused js/src/tests/lib/jittests.py:499: undefined name 'out' js/src/tests/lib/jittests.py:499: undefined name 'err' js/src/tests/lib/jittests.py:546: 'android' imported but unused It doesn't catch everything, and not everything it catches is a bug, but it can still be pretty useful.
Ah, yeah. I actually used pyflakes before. I was talking more about maybe synthesizing different kinds of test failures, so I can be sure to hit more code paths.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Dirkjan: is there anything I can help with to move this forward?
Not really. Sorry for stalling so long, I'll get back into it.
I started hacking on this again, but right now I can't figure out how to run the test suite (jstests). There's no makefile that I can use targets from, and if I try to run it directly it doesn't seem to work: djc@djc-mbp src $ python tests/jstests.py ../../obj-ff-dbg/js/src/js Traceback (most recent call last): File "tests/jstests.py", line 339, in <module> sys.exit(main()) File "tests/jstests.py", line 297, in main skip_list, test_list = load_tests(options, requested_paths, excluded_paths) File "tests/jstests.py", line 239, in load_tests test_list = manifest.load(test_dir, xul_tester) File "/Users/djc/src/mozilla-central/js/src/tests/lib/manifest.py", line 376, in load _parse_test_header(fullpath, testcase, xul_tester) File "/Users/djc/src/mozilla-central/js/src/tests/lib/manifest.py", line 277, in _parse_test_header _parse_one(testcase, xul_tester) File "/Users/djc/src/mozilla-central/js/src/tests/lib/manifest.py", line 132, in _parse_one if xul_tester.test(cond): File "/Users/djc/src/mozilla-central/js/src/tests/lib/manifest.py", line 99, in test % (cond, out, err)) Exception: Failed to test XUL condition '!xulRuntime.shell&&xulRuntime.OS=="Linux"&&xulRuntime.XPCOMABI.match(/x86_64/)'; output was '', stderr was 'dyld: Library not loaded: @executable_path/libnss3.dylib\n Referenced from: /Users/djc/src/mozilla-central/obj-ff-dbg/js/src/js\n Reason: image not found\n' If I set LD_PRELOAD_PATH to obj-ff-debug/dist/libs, it still doesn't work. Any hints?
I think setting DYLD_LIBRARY_PATH will work. DYLD_LIBRARY_PATH=../../obj-ff-dbg/dist/lib python tests/jstests.py ../../obj-ff-dbg/js/src/js By the way, how about passing "../../obj-ff-dbg/dist/bin/js" instead of "../../obj-ff-dbg/js/src/js"? The symbolic link to libnss3.dylib is also located there, so "@executable_path/libnss3.dylib" can be loaded from it without setting DYLD_LIBRARY_PATH.
I'm going to drop the pretention that I'll ever get this further than I have so far, sorry.
Assignee: dirkjan → nobody
Severity: normal → S3

Given that some patches landed and that the status of it today is that the backend of the test suite are already co-located under js/src/tests/lib.

I will close this bug, but feel free to open a new one if someone is still willing to work on it.

Blocks: sm-meta
Severity: S3 → N/A
Status: REOPENED → RESOLVED
Type: defect → enhancement
Closed: 12 years ago → 10 months ago
Priority: -- → P3
Resolution: --- → INACTIVE
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: