Closed
Bug 1073853
Opened 10 years ago
Closed 10 years ago
Various Symbol-related test failures when Gecko 35 merges to Aurora
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
VERIFIED
FIXED
mozilla35
Tracking | Status | |
---|---|---|
firefox34 | --- | unaffected |
firefox35 | --- | verified |
People
(Reporter: RyanVM, Assigned: jorendorff)
References
Details
Attachments
(1 file, 3 obsolete files)
1.80 KB,
patch
|
arai
:
review+
|
Details | Diff | Splinter Review |
Need to account for Symbols being enabled only on trunk. https://tbpl.mozilla.org/php/getParsedLog.php?id=49028577&tree=Try 20:35:01 WARNING - TEST-UNEXPECTED-FAIL | tests/jit-test/jit-test/tests/gc/bug-1053676.js | /builds/slave/test/build/tests/jit-test/jit-test/tests/gc/bug-1053676.js:8:0 ReferenceError: Symbol is not defined (code 3, args "--ion-eager") 20:35:01 INFO - INFO exit-status : 3 20:35:01 INFO - INFO timed-out : False 20:35:01 INFO - INFO stderr 2> /builds/slave/test/build/tests/jit-test/jit-test/tests/gc/bug-1053676.js:8:0 ReferenceError: Symbol is not defined 20:44:24 WARNING - TEST-UNEXPECTED-FAIL | tests/jit-test/jit-test/tests/ion/dce-with-rinstructions.js | /builds/slave/test/build/tests/jit-test/jit-test/tests/ion/dce-with-rinstructions.js:932:36 ReferenceError: Symbol is not defined (code 3, args "") 20:44:24 INFO - INFO exit-status : 3 20:44:24 INFO - INFO timed-out : False 20:44:24 INFO - INFO stderr 2> /builds/slave/test/build/tests/jit-test/jit-test/tests/ion/dce-with-rinstructions.js:932:36 ReferenceError: Symbol is not defined
Comment 1•10 years ago
|
||
Fixed following tests: js/src/jit-test/tests/gc/bug-1053676.js js/src/jit-test/tests/ion/dce-with-rinstructions.js js/src/jsapi-tests/testForOfIterator.cpp Green on try run: https://tbpl.mozilla.org/?tree=Try&rev=7acf018c08ae (Aurora Simulation) https://tbpl.mozilla.org/?tree=Try&rev=3f2210f83221 (trunk)
Attachment #8498895 -
Flags: review?(nicolas.b.pierron)
Attachment #8498895 -
Flags: review?(jorendorff)
Comment 2•10 years ago
|
||
Comment on attachment 8498895 [details] [diff] [review] Do Symbol-related tests only if defined. Review of attachment 8498895 [details] [diff] [review]: ----------------------------------------------------------------- I kind of prefer the approach of using getBuildConfiguration [1,2], to reflect the configuration choices. But I do not have a strong opinion here. [1] http://dxr.mozilla.org/mozilla-central/search?q=path%3Ajs%2Fsrc%2Fjit-test+getBuildConfiguration&case=true&offset=101 [2] http://dxr.mozilla.org/mozilla-central/source/js/src/builtin/TestingFunctions.cpp?from=getBuildConfiguration#51 ::: js/src/jit-test/tests/gc/bug-1053676.js @@ +1,2 @@ > // |jit-test| ion-eager; > +if (typeof Symbol === "function") { nit: It would be better to invert this condition, such as: if (typeof Symbol != "function") quit(0); and avoid the re-indentation.
Attachment #8498895 -
Flags: review?(nicolas.b.pierron) → review+
Comment 3•10 years ago
|
||
Thank you for reviewing! (In reply to Nicolas B. Pierron [:nbp] from comment #2) > Comment on attachment 8498895 [details] [diff] [review] > Do Symbol-related tests only if defined. > > Review of attachment 8498895 [details] [diff] [review]: > ----------------------------------------------------------------- > > I kind of prefer the approach of using getBuildConfiguration [1,2], to > reflect the configuration choices. But I do not have a strong opinion here. I guess it might be better to replace all other usage of `typeof Symbol === "function"` tests at once, to make it easier to remove them after bug 1069416 is fixed. So, I'd like to file another bug for it, okay? > ::: js/src/jit-test/tests/gc/bug-1053676.js > @@ +1,2 @@ > > // |jit-test| ion-eager; > > +if (typeof Symbol === "function") { > > nit: It would be better to invert this condition, such as: > > if (typeof Symbol != "function") quit(0); > > and avoid the re-indentation. Thanks, I'll fix it soon.
Comment 4•10 years ago
|
||
(In reply to Tooru Fujisawa [:arai] from comment #3) > (In reply to Nicolas B. Pierron [:nbp] from comment #2) > > I kind of prefer the approach of using getBuildConfiguration [1,2], to > > reflect the configuration choices. But I do not have a strong opinion here. > > I guess it might be better to replace all other usage of `typeof Symbol === > "function"` tests at once, to make it easier to remove them after bug > 1069416 is fixed. > So, I'd like to file another bug for it, okay? Sounds good to me, if Jason thinks that's good to modify getBuildConfiguration.
Comment 5•10 years ago
|
||
> if (typeof Symbol != "function") quit(0);
Updated :)
Attachment #8498895 -
Attachment is obsolete: true
Attachment #8498895 -
Flags: review?(jorendorff)
Attachment #8501383 -
Flags: review?(jorendorff)
Comment 6•10 years ago
|
||
(In reply to Nicolas B. Pierron [:nbp] from comment #4) > (In reply to Tooru Fujisawa [:arai] from comment #3) > > (In reply to Nicolas B. Pierron [:nbp] from comment #2) > > > I kind of prefer the approach of using getBuildConfiguration [1,2], to > > > reflect the configuration choices. But I do not have a strong opinion here. > > > > I guess it might be better to replace all other usage of `typeof Symbol === > > "function"` tests at once, to make it easier to remove them after bug > > 1069416 is fixed. > > So, I'd like to file another bug for it, okay? > > Sounds good to me, if Jason thinks that's good to modify > getBuildConfiguration. Please don't do this. The absence of Symbol is not expected to be a long-lived thing, only a short-term thing while symbol bugs are ironed out. There's no reason to put lipstick on the pig. Additionally, getBuildConfiguration is a SpiderMonkey shell-ism. Tests that use it aren't immediately portable to run against other engines. We should only write tests to depend on SpiderMonkey-specific behaviors when there's no other way to do it. It's easy to do it another way, here. With respect to quit(0), I would somewhat rather the test were encapsulated in a function, guarded like so: if (typeof Symbol === "function") test(); because quit(0) is SpiderMonkey-specific, but this is enough of an existing problem I won't complain too strongly if you do the quit trick.
Reporter | ||
Comment 7•10 years ago
|
||
FYI, merge day is on Monday, so let's please get *something* landed here soon so I can verify a green Try run ahead of time.
Reporter | ||
Comment 8•10 years ago
|
||
[Tracking Requested - why for this release]: Will cause perma-orange after the uplift to Aurora.
tracking-firefox35:
--- → ?
Comment 9•10 years ago
|
||
Thank you Waldo for letting me know about portability of testcase :) About "quit", I'm not sure encapsulating the test in a function does not break the condition, so, if needed, I'd like to fix it later in another bug, together with other tests. Green on try run for attachment 8501383 [details] [diff] [review]: https://tbpl.mozilla.org/?tree=Try&rev=528768f2e0a0 (trunk) Aurora build fails :/ (bug 1079573) https://tbpl.mozilla.org/?tree=Try&rev=06dea6083065 (Aurora Simulation) I'll test it after that bug is fixed. But the difference between previous try run ( https://tbpl.mozilla.org/?tree=Try&rev=7acf018c08ae ) is only in testcase, so I guess it will pass unless other Symbol-related tests are added after it. I'll ask jorendorff for reviewing later. Adding needinfo? just in case.
Flags: needinfo?(jorendorff)
Assignee | ||
Comment 10•10 years ago
|
||
Attachment #8501823 -
Flags: review?(bzbarsky)
Assignee | ||
Updated•10 years ago
|
Assignee: arai_a → jorendorff
Status: NEW → ASSIGNED
Assignee | ||
Comment 11•10 years ago
|
||
Comment on attachment 8501383 [details] [diff] [review] Do Symbol-related tests only if defined. Review of attachment 8501383 [details] [diff] [review]: ----------------------------------------------------------------- Drop the changes to testForOfIterator.cpp. Otherwise this is good to land right now.
Attachment #8501383 -
Flags: review?(jorendorff) → review+
Assignee | ||
Comment 12•10 years ago
|
||
Comment on attachment 8501823 [details] [diff] [review] Fix some tests that fail when symbols are not defined Wrong bug.
Attachment #8501823 -
Attachment is obsolete: true
Attachment #8501823 -
Flags: review?(bzbarsky)
Flags: needinfo?(jorendorff)
Comment 13•10 years ago
|
||
(In reply to Jason Orendorff [:jorendorff] from comment #11) > Comment on attachment 8501383 [details] [diff] [review] > Do Symbol-related tests only if defined. > > Review of attachment 8501383 [details] [diff] [review]: > ----------------------------------------------------------------- > > Drop the changes to testForOfIterator.cpp. Otherwise this is good to land > right now. Thank you! Try is running: https://tbpl.mozilla.org/?tree=Try&rev=cdc0efc72df6
Attachment #8501383 -
Attachment is obsolete: true
Attachment #8501846 -
Flags: review+
Comment 15•10 years ago
|
||
Sorry, I forgot about Aurora Simulation try. removing checkin-needed for now.
Keywords: checkin-needed
Comment 16•10 years ago
|
||
Try run for Aurora Simulration started: https://tbpl.mozilla.org/?tree=Try&rev=e5e1b426677a
Comment 17•10 years ago
|
||
Almost passed Aurora Simulation Try run. Remaining failure except M-e10s(bc1) will be fixed by bug 1079640. About browser_e10s_switchbrowser.js in M-e10s(bc1), I'll investigate it, but I think it's not related to this patch.
Keywords: checkin-needed
Reporter | ||
Comment 18•10 years ago
|
||
M-e10s isn't support on !trunk. You're good :)
Reporter | ||
Comment 19•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/9bdba7a66515
Flags: in-testsuite+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/9bdba7a66515
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
Reporter | ||
Updated•10 years ago
|
Status: RESOLVED → VERIFIED
Updated•10 years ago
|
tracking-firefox35:
? → ---
You need to log in
before you can comment on or make changes to this bug.
Description
•